* Unify the treatment of sources copied to the store, and recursive

  SHA-256 outputs of fixed-output derivations.  I.e. they now produce
  the same store path:

  $ nix-store --add x
  /nix/store/j2fq9qxvvxgqymvpszhs773ncci45xsj-x

  $ nix-store --add-fixed --recursive sha256 x
  /nix/store/j2fq9qxvvxgqymvpszhs773ncci45xsj-x

  the latter being the same as the path that a derivation

    derivation {
      name = "x";
      outputHashAlgo = "sha256";
      outputHashMode = "recursive";
      outputHash = "...";
      ...
    };

  produces.

  This does change the output path for such fixed-output derivations.
  Fortunately they are quite rare.  The most common use is fetchsvn
  calls with SHA-256 hashes.  (There are a handful of those is
  Nixpkgs, mostly unstable development packages.)
  
* Documented the computation of store paths (in store-api.cc).

12 files changed, 191 insertions, 79 deletions

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 4b7702effc..27ce28b8f7 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -215,6 +215,14 @@ static Expr prim_trace(EvalState & state, const ATermVector & args)
  *************************************************************/
 
 
+static bool isFixedOutput(const Derivation & drv)
+{
+    return drv.outputs.size() == 1 &&
+        drv.outputs.begin()->first == "out" &&
+        drv.outputs.begin()->second.hash != "";
+}
+
+
 /* Returns the hash of a derivation modulo fixed-output
    subderivations.  A fixed-output derivation is a derivation with one
    output (`out') for which an expected hash and hash algorithm are
@@ -227,27 +235,23 @@ static Expr prim_trace(EvalState & state, const ATermVector & args)
    function, we do not want to rebuild everything depending on it
    (after all, (the hash of) the file being downloaded is unchanged).
    So the *output paths* should not change.  On the other hand, the
-   *derivation store expression paths* should change to reflect the
-   new dependency graph.
+   *derivation paths* should change to reflect the new dependency
+   graph.
 
    That's what this function does: it returns a hash which is just the
-   of the derivation ATerm, except that any input store expression
+   hash of the derivation ATerm, except that any input derivation
    paths have been replaced by the result of a recursive call to this
-   function, and that for fixed-output derivations we return
-   (basically) its outputHash. */
+   function, and that for fixed-output derivations we return a hash of
+   its output path. */
 static Hash hashDerivationModulo(EvalState & state, Derivation drv)
 {
     /* Return a fixed hash for fixed-output derivations. */
-    if (drv.outputs.size() == 1) {
+    if (isFixedOutput(drv)) {
         DerivationOutputs::const_iterator i = drv.outputs.begin();
-        if (i->first == "out" &&
-            i->second.hash != "")
-        {
-            return hashString(htSHA256, "fixed:out:"
-                + i->second.hashAlgo + ":"
-                + i->second.hash + ":"
-                + i->second.path);
-        }
+        return hashString(htSHA256, "fixed:out:"
+            + i->second.hashAlgo + ":"
+            + i->second.hash + ":"
+            + i->second.path);
     }
 
     /* For other derivations, replace the inputs paths with recursive
@@ -304,8 +308,7 @@ static Expr prim_derivationStrict(EvalState & state, const ATermVector & args)
     
     PathSet context;
 
-    string outputHash;
-    string outputHashAlgo;
+    string outputHash, outputHashAlgo;
     bool outputHashRecursive = false;
 
     for (ATermMap::const_iterator i = attrs.begin(); i != attrs.end(); ++i) {
@@ -380,6 +383,7 @@ static Expr prim_derivationStrict(EvalState & state, const ATermVector & args)
         throw EvalError("required attribute `system' missing");
 
     /* If an output hash was given, check it. */
+    Path outPath;
     if (outputHash == "")
         outputHashAlgo = "";
     else {
@@ -398,6 +402,7 @@ static Expr prim_derivationStrict(EvalState & state, const ATermVector & args)
                 % outputHash % outputHashAlgo);
         string s = outputHash;
         outputHash = printHash(h);
+        outPath = makeFixedOutputPath(outputHashRecursive, outputHashAlgo, h, drvName);
         if (outputHashRecursive) outputHashAlgo = "r:" + outputHashAlgo;
     }
 
@@ -413,13 +418,12 @@ static Expr prim_derivationStrict(EvalState & state, const ATermVector & args)
        have an empty value.  This ensures that changes in the set of
        output names do get reflected in the hash. */
     drv.env["out"] = "";
-    drv.outputs["out"] =
-        DerivationOutput("", outputHashAlgo, outputHash);
+    drv.outputs["out"] = DerivationOutput("", outputHashAlgo, outputHash);
         
     /* Use the masked derivation expression to compute the output
        path. */
-    Path outPath = makeStorePath("output:out",
-        hashDerivationModulo(state, drv), drvName);
+    if (outPath == "")
+        outPath = makeStorePath("output:out", hashDerivationModulo(state, drv), drvName);
 
     /* Construct the final derivation store expression. */
     drv.env["out"] = outPath;
@@ -632,8 +636,8 @@ static Expr prim_filterSource(EvalState & state, const ATermVector & args)
     FilterFromExpr filter(state, args[0]);
 
     Path dstPath = readOnlyMode
-        ? computeStorePathForPath(path, false, false, "", filter).first
-        : store->addToStore(path, false, false, "", filter);
+        ? computeStorePathForPath(path, true, "sha256", filter).first
+        : store->addToStore(path, true, "sha256", filter);
 
     return makeStr(dstPath, singleton<PathSet>(dstPath));
 }
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 9dcd134711..e015894b9f 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -670,14 +670,14 @@ void LocalStore::invalidatePath(const Path & path)
 }
 
 
-Path LocalStore::addToStore(const Path & _srcPath, bool fixed,
+Path LocalStore::addToStore(const Path & _srcPath,
     bool recursive, string hashAlgo, PathFilter & filter)
 {
     Path srcPath(absPath(_srcPath));
     debug(format("adding `%1%' to the store") % srcPath);
 
     std::pair<Path, Hash> pr =
-        computeStorePathForPath(srcPath, fixed, recursive, hashAlgo, filter);
+        computeStorePathForPath(srcPath, recursive, hashAlgo, filter);
     Path & dstPath(pr.first);
     Hash & h(pr.second);
 
@@ -696,10 +696,13 @@ Path LocalStore::addToStore(const Path & _srcPath, bool fixed,
 
             copyPath(srcPath, dstPath, filter);
 
+            /* !!! */
+#if 0            
             Hash h2 = hashPath(htSHA256, dstPath, filter);
             if (h != h2)
                 throw Error(format("contents of `%1%' changed while copying it to `%2%' (%3% -> %4%)")
                     % srcPath % dstPath % printHash(h) % printHash(h2));
+#endif
 
             canonicalisePathMetaData(dstPath);
             
@@ -713,10 +716,10 @@ Path LocalStore::addToStore(const Path & _srcPath, bool fixed,
 }
 
 
-Path LocalStore::addTextToStore(const string & suffix, const string & s,
+Path LocalStore::addTextToStore(const string & name, const string & s,
     const PathSet & references)
 {
-    Path dstPath = computeStorePathForText(suffix, s, references);
+    Path dstPath = computeStorePathForText(name, s, references);
     
     addTempRoot(dstPath);
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 37ed543fc3..3d47446f6e 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -89,11 +89,11 @@ public:
     bool querySubstitutablePathInfo(const Path & substituter,
         const Path & path, SubstitutablePathInfo & info);
     
-    Path addToStore(const Path & srcPath, bool fixed = false,
-        bool recursive = false, string hashAlgo = "",
+    Path addToStore(const Path & srcPath,
+        bool recursive = true, string hashAlgo = "sha256",
         PathFilter & filter = defaultPathFilter);
 
-    Path addTextToStore(const string & suffix, const string & s,
+    Path addTextToStore(const string & name, const string & s,
         const PathSet & references);
 
     void exportPath(const Path & path, bool sign,
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 7e5fa2d865..f79b22310b 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -278,14 +278,15 @@ Path RemoteStore::queryDeriver(const Path & path)
 }
 
 
-Path RemoteStore::addToStore(const Path & _srcPath, bool fixed,
+Path RemoteStore::addToStore(const Path & _srcPath,
     bool recursive, string hashAlgo, PathFilter & filter)
 {
     Path srcPath(absPath(_srcPath));
     
     writeInt(wopAddToStore, to);
     writeString(baseNameOf(srcPath), to);
-    writeInt(fixed ? 1 : 0, to);
+    /* backwards compatibility hack */
+    writeInt((hashAlgo == "sha256" && recursive) ? 0 : 1, to);
     writeInt(recursive ? 1 : 0, to);
     writeString(hashAlgo, to);
     dumpPath(srcPath, to, filter);
@@ -294,11 +295,11 @@ Path RemoteStore::addToStore(const Path & _srcPath, bool fixed,
 }
 
 
-Path RemoteStore::addTextToStore(const string & suffix, const string & s,
+Path RemoteStore::addTextToStore(const string & name, const string & s,
     const PathSet & references)
 {
     writeInt(wopAddTextToStore, to);
-    writeString(suffix, to);
+    writeString(name, to);
     writeString(s, to);
     writeStringSet(references, to);
     
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index 969c8f9b62..cb9124a4cd 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -42,11 +42,11 @@ public:
     bool querySubstitutablePathInfo(const Path & path,
         SubstitutablePathInfo & info);
     
-    Path addToStore(const Path & srcPath, bool fixed = false,
-        bool recursive = false, string hashAlgo = "",
+    Path addToStore(const Path & srcPath,
+        bool recursive = true, string hashAlgo = "sha256",
         PathFilter & filter = defaultPathFilter);
 
-    Path addTextToStore(const string & suffix, const string & s,
+    Path addTextToStore(const string & name, const string & s,
         const PathSet & references);
 
     void exportPath(const Path & path, bool sign,
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index ced5fd0072..fe4ecfad54 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -99,55 +99,112 @@ void checkStoreName(const string & name)
 }
 
 
+/* Store paths have the following form:
+
+   <store>/<h>-<name>
+
+   where
+
+   <store> = the location of the Nix store, usually /nix/store
+   
+   <name> = a human readable name for the path, typically obtained
+     from the name attribute of the derivation, or the name of the
+     source file from which the store path is created
+     
+   <h> = base-32 representation of the first 160 bits of a SHA-256
+     hash of <s>; the hash part of the store name
+     
+   <s> = the string "<type>:sha256:<h2>:<store>:<name>";
+     note that it includes the location of the store as well as the
+     name to make sure that changes to either of those are reflected
+     in the hash (e.g. you won't get /nix/store/<h>-name1 and
+     /nix/store/<h>-name2 with equal hash parts).
+     
+   <type> = one of:
+     "text:<r1>:<r2>:...<rN>"
+       for plain text files written to the store using
+       addTextToStore(); <r1> ... <rN> are the references of the
+       path.
+     "source"
+       for paths copied to the store using addToStore() when recursive
+       = true and hashAlgo = "sha256"
+     "output:out"
+       for either the outputs created by derivations, OR paths copied
+       to the store using addToStore() with recursive != true or
+       hashAlgo != "sha256" (in that case "source" is used; it's
+       silly, but it's done that way for compatibility).
+
+   <h2> = base-16 representation of a SHA-256 hash of:
+     if <type> = "text:...":
+       the string written to the resulting store path
+     if <type> = "source":
+       the serialisation of the path from which this store path is
+       copied, as returned by hashPath()
+     if <type> = "output:out":
+       for non-fixed derivation outputs:
+         the derivation (see hashDerivationModulo() in
+         primops.cc)
+       for paths copied by addToStore() or produced by fixed-output
+       derivations:
+         the string "fixed:out:<rec><algo>:<hash>:", where
+           <rec> = "r:" for recursive (path) hashes, or "" or flat
+             (file) hashes
+           <algo> = "md5", "sha1" or "sha256"
+           <hash> = base-16 representation of the path or flat hash of
+             the contents of the path (or expected contents of the
+             path for fixed-output derivations)
+
+   It would have been nicer to handle fixed-output derivations under
+   "source", e.g. have something like "source:<rec><algo>", but we're
+   stuck with this for now...
+
+   The main reason for this way of computing names is to prevent name
+   collisions (for security).  For instance, it shouldn't be feasible
+   to come up with a derivation whose output path collides with the
+   path for a copied source.  The former would have a <s> starting with
+   "output:out:", while the latter would have a <2> starting with
+   "source:".
+*/
+
+
 Path makeStorePath(const string & type,
-    const Hash & hash, const string & suffix)
+    const Hash & hash, const string & name)
 {
     /* e.g., "source:sha256:1abc...:/nix/store:foo.tar.gz" */
     string s = type + ":sha256:" + printHash(hash) + ":"
-        + nixStore + ":" + suffix;
+        + nixStore + ":" + name;
 
-    checkStoreName(suffix);
+    checkStoreName(name);
 
     return nixStore + "/"
         + printHash32(compressHash(hashString(htSHA256, s), 20))
-        + "-" + suffix;
+        + "-" + name;
 }
 
 
 Path makeFixedOutputPath(bool recursive,
     string hashAlgo, Hash hash, string name)
 {
-    /* !!! copy/paste from primops.cc */
-    Hash h = hashString(htSHA256, "fixed:out:"
-        + (recursive ? (string) "r:" : "") + hashAlgo + ":"
-        + printHash(hash) + ":"
-        + "");
-    return makeStorePath("output:out", h, name);
+    return hashAlgo == "sha256" && recursive
+        ? makeStorePath("source", hash, name)
+        : makeStorePath("output:out", hashString(htSHA256,
+                "fixed:out:" + (recursive ? (string) "r:" : "") + hashAlgo + ":" + printHash(hash) + ":"),
+            name);
 }
 
 
 std::pair<Path, Hash> computeStorePathForPath(const Path & srcPath,
-    bool fixed, bool recursive, string hashAlgo, PathFilter & filter)
+    bool recursive, string hashAlgo, PathFilter & filter)
 {
-    Hash h = hashPath(htSHA256, srcPath, filter);
-
-    string baseName = baseNameOf(srcPath);
-
-    Path dstPath;
-    
-    if (fixed) {
-        HashType ht(parseHashType(hashAlgo));
-        Hash h2 = recursive ? hashPath(ht, srcPath, filter) : hashFile(ht, srcPath);
-        dstPath = makeFixedOutputPath(recursive, hashAlgo, h2, baseName);
-    }
-        
-    else dstPath = makeStorePath("source", h, baseName);
-
+    HashType ht(parseHashType(hashAlgo));
+    Hash h = recursive ? hashPath(ht, srcPath, filter) : hashFile(ht, srcPath);
+    string name = baseNameOf(srcPath);
+    Path dstPath = makeFixedOutputPath(recursive, hashAlgo, h, name);
     return std::pair<Path, Hash>(dstPath, h);
 }
 
 
-Path computeStorePathForText(const string & suffix, const string & s,
+Path computeStorePathForText(const string & name, const string & s,
     const PathSet & references)
 {
     Hash hash = hashString(htSHA256, s);
@@ -159,7 +216,7 @@ Path computeStorePathForText(const string & suffix, const string & s,
         type += ":";
         type += *i;
     }
-    return makeStorePath(type, hash, suffix);
+    return makeStorePath(type, hash, name);
 }
 
 
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index ed7e601460..adfd40a919 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -173,13 +173,13 @@ public:
        derivation is pre-loaded into the Nix store.  The function
        object `filter' can be used to exclude files (see
        libutil/archive.hh). */
-    virtual Path addToStore(const Path & srcPath, bool fixed = false,
-        bool recursive = false, string hashAlgo = "",
+    virtual Path addToStore(const Path & srcPath,
+        bool recursive = true, string hashAlgo = "sha256",
         PathFilter & filter = defaultPathFilter) = 0;
 
     /* Like addToStore, but the contents written to the output path is
        a regular file containing the given string. */
-    virtual Path addTextToStore(const string & suffix, const string & s,
+    virtual Path addTextToStore(const string & name, const string & s,
         const PathSet & references) = 0;
 
     /* Export a store path, that is, create a NAR dump of the store
@@ -274,7 +274,7 @@ Path followLinksToStorePath(const Path & path);
 
 /* Constructs a unique store path name. */
 Path makeStorePath(const string & type,
-    const Hash & hash, const string & suffix);
+    const Hash & hash, const string & name);
     
 Path makeFixedOutputPath(bool recursive,
     string hashAlgo, Hash hash, string name);
@@ -285,7 +285,7 @@ Path makeFixedOutputPath(bool recursive,
    Returns the store path and the cryptographic hash of the
    contents of srcPath. */
 std::pair<Path, Hash> computeStorePathForPath(const Path & srcPath,
-    bool fixed = false, bool recursive = false, string hashAlgo = "",
+    bool recursive = true, string hashAlgo = "sha256",
     PathFilter & filter = defaultPathFilter);
 
 /* Preparatory part of addTextToStore().
@@ -302,7 +302,7 @@ std::pair<Path, Hash> computeStorePathForPath(const Path & srcPath,
    simply yield a different store path, so other users wouldn't be
    affected), but it has some backwards compatibility issues (the
    hashing scheme changes), so I'm not doing that for now. */
-Path computeStorePathForText(const string & suffix, const string & s,
+Path computeStorePathForText(const string & name, const string & s,
     const PathSet & references);
 
 
diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc
index 63635b52d3..e03e822212 100644
--- a/src/nix-store/nix-store.cc
+++ b/src/nix-store/nix-store.cc
@@ -129,7 +129,7 @@ static void opAddFixed(Strings opFlags, Strings opArgs)
     opArgs.pop_front();
 
     for (Strings::iterator i = opArgs.begin(); i != opArgs.end(); ++i)
-        cout << format("%1%\n") % store->addToStore(*i, true, recursive, hashAlgo);
+        cout << format("%1%\n") % store->addToStore(*i, recursive, hashAlgo);
 }
 
 
@@ -151,6 +151,9 @@ static void opPrintFixedPath(Strings opFlags, Strings opArgs)
         if (*i == "--recursive") recursive = true;
         else throw UsageError(format("unknown flag `%1%'") % *i);
 
+    if (opArgs.size() != 3)
+        throw UsageError(format("`--print-fixed-path' requires three arguments"));
+    
     Strings::iterator i = opArgs.begin();
     string hashAlgo = *i++;
     string hash = *i++;
diff --git a/src/nix-worker/nix-worker.cc b/src/nix-worker/nix-worker.cc
index 1ba74f46fe..e4fe94926b 100644
--- a/src/nix-worker/nix-worker.cc
+++ b/src/nix-worker/nix-worker.cc
@@ -290,7 +290,7 @@ static void performOp(unsigned int clientVersion,
     case wopAddToStore: {
         /* !!! uberquick hack */
         string baseName = readString(from);
-        bool fixed = readInt(from) == 1;
+        readInt(from); /* obsolete; was `fixed' flag */
         bool recursive = readInt(from) == 1;
         string hashAlgo = readString(from);
         
@@ -300,7 +300,7 @@ static void performOp(unsigned int clientVersion,
         restorePath(tmp2, from);
 
         startWork();
-        Path path = store->addToStore(tmp2, fixed, recursive, hashAlgo);
+        Path path = store->addToStore(tmp2, recursive, hashAlgo);
         stopWork();
         
         writeString(path, to);
diff --git a/tests/add.sh b/tests/add.sh
index 83092b26c1..7ea4cb6d6a 100644
--- a/tests/add.sh
+++ b/tests/add.sh
@@ -1,13 +1,28 @@
 source common.sh
 
-file=./add.sh
+path1=$($nixstore --add ./dummy)
+echo $path1
 
-path=$($nixstore --add $file)
+path2=$($nixstore --add-fixed sha256 --recursive ./dummy)
+echo $path2
 
-echo $path
+if test "$path1" != "$path2"; then
+    echo "nix-store --add and --add-fixed mismatch"
+    exit 1
+fi    
 
-hash=$($nixstore -q --hash $path)
+path3=$($nixstore --add-fixed sha256 ./dummy)
+echo $path3
+test "$path1" != "$path3" || exit 1
 
-echo $hash
+path4=$($nixstore --add-fixed sha1 --recursive ./dummy)
+echo $path4
+test "$path1" != "$path4" || exit 1
 
-test "$hash" = "sha256:$($nixhash --type sha256 --base32 $file)"
+hash1=$($nixstore -q --hash $path1)
+echo $hash1
+
+hash2=$($nixhash --type sha256 --base32 ./dummy)
+echo $hash2
+
+test "$hash1" = "sha256:$hash2"
diff --git a/tests/fixed.nix.in b/tests/fixed.nix.in
index 5d364eb39e..ad5306cf6a 100644
--- a/tests/fixed.nix.in
+++ b/tests/fixed.nix.in
@@ -20,7 +20,6 @@ rec {
     (f ./fixed.builder1.sh "flat" "sha1" "a0b65939670bc2c010f4d5d6a0b3e4e4590fb92b")
     (f ./fixed.builder2.sh "recursive" "md5" "3670af73070fa14077ad74e0f5ea4e42")
     (f ./fixed.builder2.sh "recursive" "sha1" "vw46m23bizj4n8afrc0fj19wrp7mj3c0")
-    (f ./fixed.builder2.sh "recursive" "sha256" "1ixr6yd3297ciyp9im522dfxpqbkhcw0pylkb2aab915278fqaik")
   ];
 
   good2 = [
@@ -30,6 +29,9 @@ rec {
     (f ./fixed.builder2.sh "flat" "md5" "8ddd8be4b179a529afa5f2ffae4b9858")
   ];
 
+  sameAsAdd =
+    f ./fixed.builder2.sh "recursive" "sha256" "1ixr6yd3297ciyp9im522dfxpqbkhcw0pylkb2aab915278fqaik";
+
   bad = [
     (f ./fixed.builder1.sh "flat" "md5" "0ddd8be4b179a529afa5f2ffae4b9858")
   ];
diff --git a/tests/fixed.sh b/tests/fixed.sh
index 72038adfce..9d5de2929b 100644
--- a/tests/fixed.sh
+++ b/tests/fixed.sh
@@ -34,3 +34,30 @@ clearStore
 drvs=$($nixinstantiate fixed.nix -A parallelSame)
 echo $drvs
 $nixstore -r $drvs -j2
+
+# Fixed-output derivations with a recursive SHA-256 hash should
+# produce the same path as "nix-store --add".
+echo 'testing sameAsAdd...'
+drv=$($nixinstantiate fixed.nix -A sameAsAdd)
+echo $drv
+out=$($nixstore -r $drv)
+echo $out
+
+# This is what fixed.builder2 produces...
+rm -rf $TEST_ROOT/fixed
+mkdir $TEST_ROOT/fixed
+mkdir $TEST_ROOT/fixed/bla
+echo "Hello World!" > $TEST_ROOT/fixed/foo
+ln -s foo $TEST_ROOT/fixed/bar
+
+out2=$($nixstore --add $TEST_ROOT/fixed)
+echo $out2
+test "$out" = "$out2" || exit 1
+
+out3=$($nixstore --add-fixed --recursive sha256 $TEST_ROOT/fixed)
+echo $out3
+test "$out" = "$out3" || exit 1
+
+out4=$($nixstore --print-fixed-path --recursive sha256 "1ixr6yd3297ciyp9im522dfxpqbkhcw0pylkb2aab915278fqaik" fixed)
+echo $out4
+test "$out" = "$out4" || exit 1