Clean up a few things related to profiles (#8526)

- Greatly expand API docs

- Clean up code in misc ways

  - Instead of a complicated single loop on generations, do different
    operations in successive subsequent steps.

  - Avoid `ref` in one place where `&` is fine

  - Just return path instead of mutating an argument in `makeName`

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

src/libcmd/command.cc

@@ -239,9 +239,7 @@ void MixProfile::updateProfile(const StorePath & storePath)
     if (!store) throw Error("'--profile' is not supported for this Nix store");
     auto profile2 = absPath(*profile);
     switchLink(profile2,
-        createGeneration(
-            ref<LocalFSStore>(store),
-            profile2, storePath));
+        createGeneration(*store, profile2, storePath));
 }
 
 void MixProfile::updateProfile(const BuiltPaths & buildables)

src/libstore/profiles.cc

@@ -13,8 +13,10 @@
 namespace nix {
 
 
-/* Parse a generation name of the format
-   `<profilename>-<number>-link'. */
+/**
+ * Parse a generation name of the format
+ * `<profilename>-<number>-link'.
+ */
 static std::optional<GenerationNumber> parseName(const std::string & profileName, const std::string & name)
 {
     if (name.substr(0, profileName.size() + 1) != profileName + "-") return {};
@@ -28,7 +30,6 @@ static std::optional<GenerationNumber> parseName(const std::string & profileName
 }
 
 
-
 std::pair<Generations, std::optional<GenerationNumber>> findGenerations(Path profile)
 {
     Generations gens;
@@ -61,15 +62,16 @@ std::pair<Generations, std::optional<GenerationNumber>> findGenerations(Path pro
 }
 
 
-static void makeName(const Path & profile, GenerationNumber num,
-    Path & outLink)
+/**
+ * Create a generation name that can be parsed by `parseName()`.
+ */
+static Path makeName(const Path & profile, GenerationNumber num)
 {
-    Path prefix = fmt("%1%-%2%", profile, num);
-    outLink = prefix + "-link";
+    return fmt("%s-%s-link", profile, num);
 }
 
 
-Path createGeneration(ref<LocalFSStore> store, Path profile, StorePath outPath)
+Path createGeneration(LocalFSStore & store, Path profile, StorePath outPath)
 {
     /* The new generation number should be higher than old the
        previous ones. */
@@ -79,7 +81,7 @@ Path createGeneration(ref<LocalFSStore> store, Path profile, StorePath outPath)
     if (gens.size() > 0) {
         Generation last = gens.back();
 
-        if (readLink(last.path) == store->printStorePath(outPath)) {
+        if (readLink(last.path) == store.printStorePath(outPath)) {
             /* We only create a new generation symlink if it differs
                from the last one.
 
@@ -89,7 +91,7 @@ Path createGeneration(ref<LocalFSStore> store, Path profile, StorePath outPath)
             return last.path;
         }
 
-        num = gens.back().number;
+        num = last.number;
     } else {
         num = 0;
     }
@@ -100,9 +102,8 @@ Path createGeneration(ref<LocalFSStore> store, Path profile, StorePath outPath)
        to the permanent roots (of which the GC would have a stale
        view).  If we didn't do it this way, the GC might remove the
        user environment etc. we've just built. */
-    Path generation;
-    makeName(profile, num + 1, generation);
-    store->addPermRoot(outPath, generation);
+    Path generation = makeName(profile, num + 1);
+    store.addPermRoot(outPath, generation);
 
     return generation;
 }
@@ -117,12 +118,19 @@ static void removeFile(const Path & path)
 
 void deleteGeneration(const Path & profile, GenerationNumber gen)
 {
-    Path generation;
-    makeName(profile, gen, generation);
+    Path generation = makeName(profile, gen);
     removeFile(generation);
 }
 
-
+/**
+ * Delete a generation with dry-run mode.
+ *
+ * Like `deleteGeneration()` but:
+ *
+ *  - We log what we are going to do.
+ *
+ *  - We only actually delete if `dryRun` is false.
+ */
 static void deleteGeneration2(const Path & profile, GenerationNumber gen, bool dryRun)
 {
     if (dryRun)
@@ -150,28 +158,37 @@ void deleteGenerations(const Path & profile, const std::set<GenerationNumber> &
     }
 }
 
+/**
+ * Advanced the iterator until the given predicate `cond` returns `true`.
+ */
+static inline void iterDropUntil(Generations & gens, auto && i, auto && cond)
+{
+    for (; i != gens.rend() && !cond(*i); ++i);
+}
+
 void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bool dryRun)
 {
+    if (max == 0)
+        throw Error("Must keep at least one generation, otherwise the current one would be deleted");
+
     PathLocks lock;
     lockProfile(lock, profile);
 
-    bool fromCurGen = false;
-    auto [gens, curGen] = findGenerations(profile);
-    for (auto i = gens.rbegin(); i != gens.rend(); ++i) {
-        if (i->number == curGen) {
-            fromCurGen = true;
-            max--;
-            continue;
-        }
-        if (fromCurGen) {
-            if (max) {
-                max--;
-                continue;
-            }
+    auto [gens, _curGen] = findGenerations(profile);
+    auto curGen = _curGen;
+
+    auto i = gens.rbegin();
+
+    // Find the current generation
+    iterDropUntil(gens, i, [&](auto & g) { return g.number == curGen; });
+
+    // Skip over `max` generations, preserving them
+    for (auto keep = 0; i != gens.rend() && keep < max; ++i, ++keep);
+
+    // Delete the rest
+    for (; i != gens.rend(); ++i)
         deleteGeneration2(profile, i->number, dryRun);
 }
-    }
-}
 
 void deleteOldGenerations(const Path & profile, bool dryRun)
 {
@@ -193,23 +210,33 @@ void deleteGenerationsOlderThan(const Path & profile, time_t t, bool dryRun)
 
     auto [gens, curGen] = findGenerations(profile);
 
-    bool canDelete = false;
-    for (auto i = gens.rbegin(); i != gens.rend(); ++i)
-        if (canDelete) {
-            assert(i->creationTime < t);
+    auto i = gens.rbegin();
+
+    // Predicate that the generation is older than the given time.
+    auto older = [&](auto & g) { return g.creationTime < t; };
+
+    // Find the first older generation, if one exists
+    iterDropUntil(gens, i, older);
+
+    /* Take the previous generation
+
+       We don't want delete this one yet because it
+       existed at the requested point in time, and
+       we want to be able to roll back to it. */
+    if (i != gens.rend()) ++i;
+
+    // Delete all previous generations (unless current).
+    for (; i != gens.rend(); ++i) {
+        /* Creating date and generations should be monotonic, so lower
+           numbered derivations should also be older. */
+        assert(older(*i));
         if (i->number != curGen)
             deleteGeneration2(profile, i->number, dryRun);
-        } else if (i->creationTime < t) {
-            /* We may now start deleting generations, but we don't
-               delete this generation yet, because this generation was
-               still the one that was active at the requested point in
-               time. */
-            canDelete = true;
     }
 }
 
 
-void deleteGenerationsOlderThan(const Path & profile, std::string_view timeSpec, bool dryRun)
+time_t parseOlderThanTimeSpec(std::string_view timeSpec)
 {
     if (timeSpec.empty() || timeSpec[timeSpec.size() - 1] != 'd')
         throw UsageError("invalid number of days specifier '%1%', expected something like '14d'", timeSpec);
@@ -221,9 +248,7 @@ void deleteGenerationsOlderThan(const Path & profile, std::string_view timeSpec,
     if (!days || *days < 1)
         throw UsageError("invalid number of days specifier '%1%'", timeSpec);
 
-    time_t oldTime = curTime - *days * 24 * 3600;
-
-    deleteGenerationsOlderThan(profile, oldTime, dryRun);
+    return curTime - *days * 24 * 3600;
 }
 
 

src/libstore/profiles.hh

@@ -1,5 +1,9 @@
 #pragma once
-///@file
+/**
+ * @file Implementation of Profiles.
+ *
+ * See the manual for additional information.
+ */
 
 #include "types.hh"
 #include "pathlocks.hh"
@@ -12,41 +16,166 @@ namespace nix {
 class StorePath;
 
 
+/**
+ * A positive number identifying a generation for a given profile.
+ *
+ * Generation numbers are assigned sequentially. Each new generation is
+ * assigned 1 + the current highest generation number.
+ */
 typedef uint64_t GenerationNumber;
 
+/**
+ * A generation is a revision of a profile.
+ *
+ * Each generation is a mapping (key-value pair) from an identifier
+ * (`number`) to a store object (specified by `path`).
+ */
 struct Generation
 {
+    /**
+     * The number of a generation is its unique identifier within the
+     * profile.
+     */
     GenerationNumber number;
+    /**
+     * The store path identifies the store object that is the contents
+     * of the generation.
+     *
+     * These store paths / objects are not unique to the generation
+     * within a profile. Nix tries to ensure successive generations have
+     * distinct contents to avoid bloat, but nothing stops two
+     * non-adjacent generations from having the same contents.
+     *
+     * @todo Use `StorePath` instead of `Path`?
+     */
     Path path;
+
+    /**
+     * When the generation was created. This is extra metadata about the
+     * generation used to make garbage collecting old generations more
+     * convenient.
+     */
     time_t creationTime;
 };
 
+/**
+ * All the generations of a profile
+ */
 typedef std::list<Generation> Generations;
 
 
 /**
- * Returns the list of currently present generations for the specified
- * profile, sorted by generation number. Also returns the number of
- * the current generation.
+ * Find all generations for the given profile.
+ *
+ * @param profile A profile specified by its name and location combined
+ * into a path. E.g. if "foo" is the name of the profile, and "/bar/baz"
+ * is the directory it is in, then the path "/bar/baz/foo" would be the
+ * argument for this parameter.
+ *
+ * @return The pair of:
+ *
+ *   - The list of currently present generations for the specified profile,
+ *     sorted by ascending generation number.
+ *
+ *   - The number of the current/active generation.
+ *
+ * Note that the current/active generation need not be the latest one.
  */
 std::pair<Generations, std::optional<GenerationNumber>> findGenerations(Path profile);
 
 class LocalFSStore;
 
-Path createGeneration(ref<LocalFSStore> store, Path profile, StorePath outPath);
+/**
+ * Create a new generation of the given profile
+ *
+ * If the previous generation (not the currently active one!) has a
+ * distinct store object, a fresh generation number is mapped to the
+ * given store object, referenced by path. Otherwise, the previous
+ * generation is assumed.
+ *
+ * The behavior of reusing existing generations like this makes this
+ * procedure idempotent. It also avoids clutter.
+ */
+Path createGeneration(LocalFSStore & store, Path profile, StorePath outPath);
 
+/**
+ * Unconditionally delete a generation
+ *
+ * @param profile A profile specified by its name and location combined into a path.
+ *
+ * @param gen The generation number specifying exactly which generation
+ * to delete.
+ *
+ * Because there is no check of whether the generation to delete is
+ * active, this is somewhat unsafe.
+ *
+ * @todo Should we expose this at all?
+ */
 void deleteGeneration(const Path & profile, GenerationNumber gen);
 
+/**
+ * Delete the given set of generations.
+ *
+ * @param profile The profile, specified by its name and location combined into a path, whose generations we want to delete.
+ *
+ * @param gensToDelete The generations to delete, specified by a set of
+ * numbers.
+ *
+ * @param dryRun Log what would be deleted instead of actually doing
+ * so.
+ *
+ * Trying to delete the currently active generation will fail, and cause
+ * no generations to be deleted.
+ */
 void deleteGenerations(const Path & profile, const std::set<GenerationNumber> & gensToDelete, bool dryRun);
 
+/**
+ * Delete generations older than `max` passed the current generation.
+ *
+ * @param profile The profile, specified by its name and location combined into a path, whose generations we want to delete.
+ *
+ * @param max How many generations to keep up to the current one. Must
+ * be at least 1 so we don't delete the current one.
+ *
+ * @param dryRun Log what would be deleted instead of actually doing
+ * so.
+ */
 void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bool dryRun);
 
+/**
+ * Delete all generations other than the current one
+ *
+ * @param profile The profile, specified by its name and location combined into a path, whose generations we want to delete.
+ *
+ * @param dryRun Log what would be deleted instead of actually doing
+ * so.
+ */
 void deleteOldGenerations(const Path & profile, bool dryRun);
 
+/**
+ * Delete generations older than `t`, except for the most recent one
+ * older than `t`.
+ *
+ * @param profile The profile, specified by its name and location combined into a path, whose generations we want to delete.
+ *
+ * @param dryRun Log what would be deleted instead of actually doing
+ * so.
+ */
 void deleteGenerationsOlderThan(const Path & profile, time_t t, bool dryRun);
 
-void deleteGenerationsOlderThan(const Path & profile, std::string_view timeSpec, bool dryRun);
+/**
+ * Parse a temp spec intended for `deleteGenerationsOlderThan()`.
+ *
+ * Throws an exception if `timeSpec` fails to parse.
+ */
+time_t parseOlderThanTimeSpec(std::string_view timeSpec);
 
+/**
+ * Smaller wrapper around `replaceSymlink` for replacing the current
+ * generation of a profile. Does not enforce proper structure.
+ *
+ * @todo Always use `switchGeneration()` instead, and delete this.
+ */
 void switchLink(Path link, Path target);
 
 /**

src/nix-collect-garbage/nix-collect-garbage.cc

@@ -41,9 +41,10 @@ void removeOldGenerations(std::string dir)
             }
             if (link.find("link") != std::string::npos) {
                 printInfo("removing old generations of profile %s", path);
-                if (deleteOlderThan != "")
-                    deleteGenerationsOlderThan(path, deleteOlderThan, dryRun);
-                else
+                if (deleteOlderThan != "") {
+                    auto t = parseOlderThanTimeSpec(deleteOlderThan);
+                    deleteGenerationsOlderThan(path, t, dryRun);
+                } else
                     deleteOldGenerations(path, dryRun);
             }
         } else if (type == DT_DIR) {

src/nix-env/nix-env.cc

@@ -772,7 +772,7 @@ static void opSet(Globals & globals, Strings opFlags, Strings opArgs)
 
     debug("switching to new user environment");
     Path generation = createGeneration(
-        ref<LocalFSStore>(store2),
+        *store2,
         globals.profile,
         drv.queryOutPath());
     switchLink(globals.profile, generation);
@@ -1356,13 +1356,14 @@ static void opDeleteGenerations(Globals & globals, Strings opFlags, Strings opAr
     if (opArgs.size() == 1 && opArgs.front() == "old") {
         deleteOldGenerations(globals.profile, globals.dryRun);
     } else if (opArgs.size() == 1 && opArgs.front().find('d') != std::string::npos) {
-        deleteGenerationsOlderThan(globals.profile, opArgs.front(), globals.dryRun);
+        auto t = parseOlderThanTimeSpec(opArgs.front());
+        deleteGenerationsOlderThan(globals.profile, t, globals.dryRun);
     } else if (opArgs.size() == 1 && opArgs.front().find('+') != std::string::npos) {
         if (opArgs.front().size() < 2)
             throw Error("invalid number of generations '%1%'", opArgs.front());
         auto str_max = opArgs.front().substr(1);
         auto max = string2Int<GenerationNumber>(str_max);
-        if (!max || *max == 0)
+        if (!max)
             throw Error("invalid number of generations to keep '%1%'", opArgs.front());
         deleteGenerationsGreaterThan(globals.profile, *max, globals.dryRun);
     } else {

src/nix-env/user-env.cc

@@ -158,7 +158,7 @@ bool createUserEnv(EvalState & state, DrvInfos & elems,
         }
 
         debug("switching to new user environment");
-        Path generation = createGeneration(ref<LocalFSStore>(store2), profile, topLevelOut);
+        Path generation = createGeneration(*store2, profile, topLevelOut);
         switchLink(profile, generation);
     }
 

src/nix/profile.cc

@@ -806,9 +806,10 @@ struct CmdProfileWipeHistory : virtual StoreCommand, MixDefaultProfile, MixDryRu
 
     void run(ref<Store> store) override
     {
-        if (minAge)
-            deleteGenerationsOlderThan(*profile, *minAge, dryRun);
-        else
+        if (minAge) {
+            auto t = parseOlderThanTimeSpec(*minAge);
+            deleteGenerationsOlderThan(*profile, t, dryRun);
+        } else
             deleteOldGenerations(*profile, dryRun);
     }
 };