Implement --replace|-r mode for pkg-unpack; improve in pkg-fetch

7 files changed, 174 insertions, 108 deletions

diff --git a/bpkg/pkg-fetch-options.cli b/bpkg/pkg-fetch-options.cli
index 4cfeb35..a064da5 100644
--- a/bpkg/pkg-fetch-options.cli
+++ b/bpkg/pkg-fetch-options.cli
@@ -17,9 +17,9 @@ bpkg pkg-fetch [<options>] (<pkg>/<ver>)|(-e <file>)"
 The \cb{pkg-fetch} command fetches the archive for the specified package
 name and version from one of the configuration's repositories. If the
 \cb{-r|--replace} option is specified, then \cb{pkg-fetch} will replace the
-archive of a package that is already in the \cb{fetched} state. Otherwise,
-\cb{pkg-fetch} expects the package to not exist in the configuration's
-database.
+archive of a package that is already in the \cb{fetched} or \cb{unpacked}
+state. Otherwise, \cb{pkg-fetch} expects the package to not exist in the
+configuration's database.
 
 If the \cb{-e|--existing} option is used, then instead of the name and
 version argument, \cb{pkg-fetch} expects a local path to the existing
@@ -36,7 +36,7 @@ namespace bpkg
   {
     bool --replace|-r
     {
-      "Replace the archive if the package is already fetched."
+      "Replace the archive if the package is already fetched or unpacked."
     };
 
     bool --existing|-e
diff --git a/bpkg/pkg-fetch.cxx b/bpkg/pkg-fetch.cxx
index c8b8543..fe8eca7 100644
--- a/bpkg/pkg-fetch.cxx
+++ b/bpkg/pkg-fetch.cxx
@@ -52,21 +52,24 @@ namespace bpkg
     {
       shared_ptr<selected_package> p (db.find<selected_package> (n));
 
-      if (p == nullptr ||
-          (p->state == package_state::fetched && o.replace ()))
-        return p;
-
+      if (p != nullptr)
       {
-        diag_record dr (error);
+        bool s (p->state == package_state::fetched ||
+                p->state == package_state::unpacked);
+
+        if (!o.replace () || !s)
+        {
+          diag_record dr (fail);
 
-        dr << "package " << n << " already exists in configuration " << c <<
-          info << "version: " << p->version << ", state: " << p->state;
+          dr << "package " << n << " already exists in configuration " << c <<
+            info << "version: " << p->version << ", state: " << p->state;
 
-        if (p->state == package_state::fetched)
-          dr << info << "use 'pkg-fetch --replace|-r' to replace its archive";
+          if (s) // Suitable state for replace?
+            dr << info << "use 'pkg-fetch --replace|-r' to replace";
+        }
       }
 
-      throw failed ();
+      return p;
     };
 
     if (o.existing ())
@@ -167,14 +170,14 @@ namespace bpkg
 
     if (sp != nullptr)
     {
-      // Clean up the archive we are replacing. Once this is done, there
-      // is no going back. If things go badly, we can't simply abort the
-      // transaction.
+      // Clean up the source directory and archive of the package we are
+      // replacing. Once this is done, there is no going back. If things
+      // go badly, we can't simply abort the transaction.
       //
-      if (sp->purge_archive)
-        pkg_purge_archive (c, t, sp);
+      pkg_purge_fs (c, t, sp);
 
       sp->version = move (m.version);
+      sp->state = package_state::fetched;
       sp->repository = move (rl);
       sp->archive = move (a);
       sp->purge_archive = purge;
diff --git a/bpkg/pkg-purge b/bpkg/pkg-purge
index 4765e60..0c88e19 100644
--- a/bpkg/pkg-purge
+++ b/bpkg/pkg-purge
@@ -14,13 +14,15 @@ namespace bpkg
   void
   pkg_purge (const pkg_purge_options&, cli::scanner& args);
 
-  // Remove package archive. If this fails, set the package state to
-  // broken, commit the transaction, and fail.
+  // Remove package's filesystem objects (the source directory and, if
+  // the archive argument is true, the package archive). If this fails,
+  // set the package state to broken, commit the transaction, and fail.
   //
   void
-  pkg_purge_archive (const dir_path& configuration,
-                     transaction&,
-                     const shared_ptr<selected_package>&);
+  pkg_purge_fs (const dir_path& configuration,
+                transaction&,
+                const shared_ptr<selected_package>&,
+                bool archive = true);
 }
 
 #endif // BPKG_PKG_PURGE
diff --git a/bpkg/pkg-purge.cxx b/bpkg/pkg-purge.cxx
index 91c4c5b..dd92857 100644
--- a/bpkg/pkg-purge.cxx
+++ b/bpkg/pkg-purge.cxx
@@ -17,26 +17,42 @@ using namespace butl;
 namespace bpkg
 {
   void
-  pkg_purge_archive (const dir_path& c,
-                     transaction& t,
-                     const shared_ptr<selected_package>& p)
+  pkg_purge_fs (const dir_path& c,
+                transaction& t,
+                const shared_ptr<selected_package>& p,
+                bool archive)
   {
-    assert (p->purge_archive && p->state != package_state::broken);
+    assert (p->state == package_state::fetched ||
+            p->state == package_state::unpacked);
 
     tracer trace ("pkg_purge_archive");
 
     database& db (t.database ());
     tracer_guard tg (db, trace);
 
-    path a (p->archive->absolute () ? *p->archive : c / *p->archive);
-
     try
     {
-      if (exists (a))
-        rm (a);
+      if (p->purge_src)
+      {
+        dir_path d (p->src_root->absolute () ? *p->src_root : c / *p->src_root);
+
+        if (exists (d)) // Don't complain if someone did our job for us.
+          rm_r (d);
+
+        p->src_root = nullopt;
+        p->purge_src = false;
+      }
+
+      if (p->purge_archive && archive)
+      {
+        path a (p->archive->absolute () ? *p->archive : c / *p->archive);
 
-      p->archive = nullopt;
-      p->purge_archive = false;
+        if (exists (a))
+          rm (a);
+
+        p->archive = nullopt;
+        p->purge_archive = false;
+      }
     }
     catch (const failed&)
     {
@@ -108,66 +124,30 @@ namespace bpkg
       }
     }
 
-    // First clean up the package source directory.
+    // For a broken package we just verify that all the filesystem objects
+    // were cleaned up by the user.
     //
-    if (p->purge_src)
+    if (p->state == package_state::broken)
     {
-      dir_path d (p->src_root->absolute () ? *p->src_root : c / *p->src_root);
-
-      if (p->state != package_state::broken)
+      if (p->out_root)
       {
-        try
-        {
-          if (exists (d)) // Don't complain if someone did our job for us.
-            rm_r (d);
-
-          p->src_root = nullopt;
-          p->purge_src = false;
-        }
-        catch (const failed&)
-        {
-          p->state = package_state::broken;
-          db.update (p);
-          t.commit ();
-
-          info << "package " << n << " is now broken; "
-               << "use 'pkg-purge --force' to remove";
-          throw;
-        }
+        dir_path d (c / *p->out_root); // Always relative.
+
+        if (exists (d))
+          fail << "broken package " << n << " output directory still exists" <<
+            info << "remove " << d << " manually then re-run pkg-purge";
       }
-      else
+
+      if (p->purge_src)
       {
-        // If we are broken, simply make sure the user cleaned things up
-        // manually.
-        //
+        dir_path d (p->src_root->absolute () ? *p->src_root : c / *p->src_root);
+
         if (exists (d))
           fail << "broken package " << n << " source directory still exists" <<
             info << "remove " << d << " manually then re-run pkg-purge";
       }
-    }
-
-    // Also check the output directory of broken packages.
-    //
-    if (p->out_root)
-    {
-      // Can only be present if broken.
-      //
-      assert (p->state == package_state::broken);
 
-      dir_path d (c / *p->out_root); // Always relative.
-
-      if (exists (d))
-        fail << "broken package " << n << " output directory still exists" <<
-          info << "remove " << d << " manually then re-run pkg-purge";
-    }
-
-    // Now the archive. Pretty much the same code as above but for a file.
-    //
-    if (p->purge_archive && !o.keep ())
-    {
-      if (p->state != package_state::broken)
-        pkg_purge_archive (c, t, p);
-      else
+      if (p->purge_archive)
       {
         path a (p->archive->absolute () ? *p->archive : c / *p->archive);
 
@@ -176,7 +156,14 @@ namespace bpkg
             info << "remove " << a << " manually then re-run pkg-purge";
       }
     }
+    else
+    {
+      assert (!p->out_root);
+      pkg_purge_fs (c, t, p, !o.keep ());
+    }
 
+    // Finally, update the database state.
+    //
     if (o.keep ())
     {
       if (p->state != package_state::fetched) // No-op we were talking about.
diff --git a/bpkg/pkg-unpack-options.cli b/bpkg/pkg-unpack-options.cli
index c9101a9..97386a1 100644
--- a/bpkg/pkg-unpack-options.cli
+++ b/bpkg/pkg-unpack-options.cli
@@ -22,7 +22,12 @@ a local path to the existing package source directory. In this case,
 configuration or package cache directories. Also, unless the
 \cb{-p|--purge} option is specified, \cb{bpkg} will not attempt to
 remove this directory when the package is purged with the \cb{pkg-purge}
-command."
+command.
+
+If \cb{-e|--existing} is specified together with the \cb{-r|--replace}
+option, then \cb{pkg-unpack} will replace the archive and/or source
+directory of a package that is already in the \cb{unpacked} or
+\cb{fetched} state."
 */
 
 namespace bpkg
@@ -39,5 +44,11 @@ namespace bpkg
     {
       "Remove the existing package directory when the package is purged."
     };
+
+    bool --replace|-r
+    {
+      "Replace the source directory if the package is already unpacked or
+       fetched. Can only be specified together with \cb{-e|--existing}."
+    };
   };
 }
diff --git a/bpkg/pkg-unpack.cxx b/bpkg/pkg-unpack.cxx
index a7c53b6..7100809 100644
--- a/bpkg/pkg-unpack.cxx
+++ b/bpkg/pkg-unpack.cxx
@@ -15,6 +15,7 @@
 #include <bpkg/database>
 #include <bpkg/diagnostics>
 
+#include <bpkg/pkg-purge>
 #include <bpkg/pkg-verify>
 
 using namespace std;
@@ -23,7 +24,11 @@ using namespace butl;
 namespace bpkg
 {
   static shared_ptr<selected_package>
-  pkg_unpack (database& db, const dir_path& c, const dir_path& d, bool purge)
+  pkg_unpack (database& db,
+              const dir_path& c,
+              const dir_path& d,
+              bool replace,
+              bool purge)
   {
     tracer trace ("pkg_unpack(dir)");
     tracer_guard tg (db, trace);
@@ -36,16 +41,6 @@ namespace bpkg
     package_manifest m (pkg_verify (d));
     level4 ([&]{trace << d << ": " << m.name << " " << m.version;});
 
-    const auto& n (m.name);
-
-    transaction t (db.begin ());
-
-    // See if this package already exists in this configuration.
-    //
-    if (shared_ptr<selected_package> p = db.find<selected_package> (n))
-      fail << "package " << n << " already exists in configuration " << c <<
-        info << "version: " << p->version << ", state: " << p->state;
-
     // Make the package and configuration paths absolute and normalized.
     // If the package is inside the configuration, use the relative path.
     // This way we can move the configuration around.
@@ -57,25 +52,63 @@ namespace bpkg
     if (ad.sub (ac))
       ad = ad.leaf (ac);
 
-    // Add the package to the configuration. Use the special root
-    // repository as the repository of this package.
+    transaction t (db.begin ());
+
+    // See if this package already exists in this configuration.
     //
-    shared_ptr<selected_package> p (new selected_package {
+    const string& n (m.name);
+    shared_ptr<selected_package> p (db.find<selected_package> (n));
+
+    if (p != nullptr)
+    {
+      bool s (p->state == package_state::fetched ||
+              p->state == package_state::unpacked);
+
+      if (!replace || !s)
+      {
+        diag_record dr (fail);
+
+        dr << "package " << n << " already exists in configuration " << c <<
+          info << "version: " << p->version << ", state: " << p->state;
+
+        if (s) // Suitable state for replace?
+          dr << info << "use 'pkg-unpack --replace|-r' to replace";
+      }
+
+      // Clean up the source directory and archive of the package we are
+      // replacing. Once this is done, there is no going back. If things
+      // go badly, we can't simply abort the transaction.
+      //
+      pkg_purge_fs (c, t, p);
+
+      // Use the special root repository as the repository of this package.
+      //
+      p->version = move (m.version);
+      p->state = package_state::unpacked;
+      p->repository = repository_location ();
+      p->src_root = move (ad);
+      p->purge_src = purge;
+
+      db.update (p);
+    }
+    else
+    {
+      p.reset (new selected_package {
         move (m.name),
         move (m.version),
         package_state::unpacked,
-        repository_location (),
+        repository_location (), // Root repository.
         nullopt,    // No archive
         false,      // Don't purge archive.
         move (ad),
         purge,
         nullopt,    // No output directory yet.
-        {}          // No prerequisites captured yet.
-     });
+        {}});       // No prerequisites captured yet.
 
-    db.persist (p);
-    t.commit ();
+      db.persist (p);
+    }
 
+    t.commit ();
     return p;
   }
 
@@ -184,6 +217,9 @@ namespace bpkg
   {
     tracer trace ("pkg_unpack");
 
+    if (o.replace () && !o.existing ())
+      fail << "-r|--replace can only be specified with -e|--existing";
+
     const dir_path& c (o.directory ());
     level4 ([&]{trace << "configuration: " << c;});
 
@@ -199,7 +235,8 @@ namespace bpkg
         fail << "package directory argument expected" <<
           info << "run 'bpkg help pkg-unpack' for more information";
 
-      p = pkg_unpack (db, c, dir_path (args.next ()), o.purge ());
+      p = pkg_unpack (
+        db, c, dir_path (args.next ()), o.replace (), o.purge ());
     }
     else
     {
diff --git a/bpkg/test.sh b/bpkg/test.sh
index f787baa..4a99347 100755
--- a/bpkg/test.sh
+++ b/bpkg/test.sh
@@ -212,9 +212,11 @@ test pkg-purge libfoo
 test pkg-fetch -e ../tests/repository/1/fetch/t1/libfoo-1.0.0.tar.gz
 stat libfoo/1.0.0 fetched
 test pkg-unpack libfoo
-fail pkg-fetch -r libfoo/1.0.0
-fail pkg-fetch -r -e ../tests/repository/1/fetch/t1/libfoo-1.0.0.tar.gz
-test pkg-purge -k libfoo
+test pkg-fetch -r libfoo/1.1.0
+stat libfoo/1.1.0 fetched
+test pkg-unpack libfoo
+test pkg-fetch -r -e ../tests/repository/1/fetch/t1/libfoo-1.0.0.tar.gz
+stat libfoo/1.0.0 fetched
 test pkg-fetch -r libfoo/1.1.0
 stat libfoo/1.1.0 fetched
 test pkg-fetch -r -e ../tests/repository/1/fetch/t1/libfoo-1.0.0.tar.gz
@@ -231,6 +233,7 @@ test pkg-fetch libhello/1.0.0
 test pkg-unpack libhello
 test pkg-purge libhello
 
+
 ## @@
 ##
 ##
@@ -246,6 +249,29 @@ stat fetched
 test pkg-purge $pkg
 stat unknown
 
+##
+## pkg-unpack
+##
+
+# @@ TODO
+
+# replace
+#
+test cfg-create --wipe
+test rep-add ../tests/repository/1/fetch/t1
+test rep-fetch
+test pkg-fetch libfoo/1.0.0
+fail pkg-unpack -e ../tests/repository/1/fetch/libfoo-1.1.0
+test pkg-unpack -r -e ../tests/repository/1/fetch/libfoo-1.1.0
+stat libfoo/1.1.0 unpacked
+test pkg-purge libfoo
+test pkg-fetch libfoo/1.0.0
+test pkg-unpack libfoo
+fail pkg-unpack -e ../tests/repository/1/fetch/libfoo-1.1.0
+test pkg-unpack -r -e ../tests/repository/1/fetch/libfoo-1.1.0
+stat libfoo/1.1.0 unpacked
+test pkg-purge libfoo
+
 
 ##
 ## pkg-purge