Split `OutputsSpec::merge` into `OuputsSpec::{union_, isSubsetOf}`

Additionally get rid of the evil time we made an empty
`OutputSpec::Names()`.

4 files changed, 56 insertions, 30 deletions

diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc
index d73109873..5090ea6d2 100644
--- a/src/libcmd/installables.cc
+++ b/src/libcmd/installables.cc
@@ -469,20 +469,14 @@ struct InstallableAttrPath : InstallableValue
 
         // Backward compatibility hack: group results by drvPath. This
         // helps keep .all output together.
-        std::map<StorePath, DerivedPath::Built> byDrvPath;
+        std::map<StorePath, OutputsSpec> byDrvPath;
 
         for (auto & drvInfo : drvInfos) {
             auto drvPath = drvInfo.queryDrvPath();
             if (!drvPath)
                 throw Error("'%s' is not a derivation", what());
 
-            auto derivedPath = byDrvPath.emplace(*drvPath, DerivedPath::Built {
-                .drvPath = *drvPath,
-                // Not normally legal, but we will merge right below
-                .outputs = OutputsSpec::Names { StringSet { } },
-            }).first;
-
-            derivedPath->second.outputs.merge(std::visit(overloaded {
+            auto newOutputs = std::visit(overloaded {
                 [&](const ExtendedOutputsSpec::Default & d) -> OutputsSpec {
                     std::set<std::string> outputsToInstall;
                     for (auto & output : drvInfo.queryOutputs(false, true))
@@ -492,12 +486,22 @@ struct InstallableAttrPath : InstallableValue
                 [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
                     return e;
                 },
-            }, extendedOutputsSpec.raw()));
+            }, extendedOutputsSpec.raw());
+
+            auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs);
+
+            if (!didInsert)
+                iter->second = iter->second.union_(newOutputs);
         }
 
         DerivedPathsWithInfo res;
-        for (auto & [_, info] : byDrvPath)
-            res.push_back({ .path = { info } });
+        for (auto & [drvPath, outputs] : byDrvPath)
+            res.push_back({
+                .path = DerivedPath::Built {
+                    .drvPath = drvPath,
+                    .outputs = outputs,
+                },
+            });
 
         return res;
     }
diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index 61169635a..2021d0023 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -144,9 +144,10 @@ void DerivationGoal::work()
 
 void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 {
-    bool newOutputs = wantedOutputs.merge(outputs);
-    if (newOutputs)
+    auto newWanted = wantedOutputs.union_(outputs);
+    if (!newWanted.isSubsetOf(wantedOutputs))
         needRestart = true;
+    wantedOutputs = newWanted;
 }
 
 
diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc
index 8e6e40c2b..d0f39a854 100644
--- a/src/libstore/outputs-spec.cc
+++ b/src/libstore/outputs-spec.cc
@@ -96,24 +96,20 @@ std::string ExtendedOutputsSpec::to_string() const
 }
 
 
-bool OutputsSpec::merge(const OutputsSpec & that)
+OutputsSpec OutputsSpec::union_(const OutputsSpec & that) const
 {
     return std::visit(overloaded {
-        [&](OutputsSpec::All &) {
-            /* If we already refer to all outputs, there is nothing to do. */
-            return false;
+        [&](const OutputsSpec::All &) -> OutputsSpec {
+            return OutputsSpec::All { };
         },
-        [&](OutputsSpec::Names & theseNames) {
+        [&](const OutputsSpec::Names & theseNames) -> OutputsSpec {
             return std::visit(overloaded {
-                [&](const OutputsSpec::All &) {
-                    *this = OutputsSpec::All {};
-                    return true;
+                [&](const OutputsSpec::All &) -> OutputsSpec {
+                    return OutputsSpec::All {};
                 },
-                [&](const OutputsSpec::Names & thoseNames) {
-                    bool ret = false;
-                    for (auto & i : thoseNames)
-                        if (theseNames.insert(i).second)
-                            ret = true;
+                [&](const OutputsSpec::Names & thoseNames) -> OutputsSpec {
+                    OutputsSpec::Names ret = theseNames;
+                    ret.insert(thoseNames.begin(), thoseNames.end());
                     return ret;
                 },
             }, that.raw());
@@ -121,6 +117,30 @@ bool OutputsSpec::merge(const OutputsSpec & that)
     }, raw());
 }
 
+
+bool OutputsSpec::isSubsetOf(const OutputsSpec & that) const
+{
+    return std::visit(overloaded {
+        [&](const OutputsSpec::All &) {
+            return true;
+        },
+        [&](const OutputsSpec::Names & thoseNames) {
+            return std::visit(overloaded {
+                [&](const OutputsSpec::All &) {
+                    return false;
+                },
+                [&](const OutputsSpec::Names & theseNames) {
+                    bool ret = true;
+                    for (auto & o : theseNames)
+                        if (thoseNames.count(o) == 0)
+                            ret = false;
+                    return ret;
+                },
+            }, raw());
+        },
+    }, that.raw());
+}
+
 }
 
 namespace nlohmann {
diff --git a/src/libstore/outputs-spec.hh b/src/libstore/outputs-spec.hh
index 9211a4fc6..82dfad479 100644
--- a/src/libstore/outputs-spec.hh
+++ b/src/libstore/outputs-spec.hh
@@ -49,10 +49,11 @@ struct OutputsSpec : _OutputsSpecRaw {
 
     bool contains(const std::string & output) const;
 
-    /* Modify the receiver outputs spec so it is the union of it's old value
-       and the argument. Return whether the output spec needed to be modified
-       --- if it didn't it was already "large enough". */
-    bool merge(const OutputsSpec & outputs);
+    /* Create a new OutputsSpec which is the union of this and that. */
+    OutputsSpec union_(const OutputsSpec & that) const;
+
+    /* Whether this OutputsSpec is a subset of that. */
+    bool isSubsetOf(const OutputsSpec & outputs) const;
 
     /* Parse a string of the form 'output1,...outputN' or
        '*', returning the outputs spec. */