Fix pkg-fetch which failed to re-fetch same package version in --replace mode

8 files changed, 132 insertions, 9 deletions

diff --git a/bpkg/package.hxx b/bpkg/package.hxx
index 02d2b07..947393b 100644
--- a/bpkg/package.hxx
+++ b/bpkg/package.hxx
@@ -1318,6 +1318,18 @@ namespace bpkg
     std::string
     string (database&) const;
 
+    // Return the relative archive path completed using the configuration
+    // directory. Return the absolute archive path as is.
+    //
+    path
+    effective_archive (const dir_path& configuration) const
+    {
+      // Cast for compiling with ODB (see above).
+      //
+      assert (static_cast<bool> (archive));
+      return archive->absolute () ? *archive : configuration / *archive;
+    }
+
     // Return the relative source directory completed using the configuration
     // directory. Return the absolute source directory as is.
     //
diff --git a/bpkg/pkg-fetch.cxx b/bpkg/pkg-fetch.cxx
index 009ddf9..837c968 100644
--- a/bpkg/pkg-fetch.cxx
+++ b/bpkg/pkg-fetch.cxx
@@ -44,17 +44,24 @@ namespace bpkg
     //
     normalize (a, "archive");
 
+    // Only purge the existing archive if its path differs from the new path.
+    //
+    shared_ptr<selected_package> p (db.find<selected_package> (n));
+
+    bool purge_archive (p != nullptr &&
+                        p->archive   &&
+                        p->effective_archive (db.config) != a);
+
     if (a.sub (db.config))
       a = a.leaf (db.config);
 
-    shared_ptr<selected_package> p (db.find<selected_package> (n));
     if (p != nullptr)
     {
       // 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.
+      // replacing. Once this is done, there is no going back. If things go
+      // badly, we can't simply abort the transaction.
       //
-      pkg_purge_fs (db, t, p, simulate);
+      pkg_purge_fs (db, t, p, simulate, purge_archive);
 
       // Note that if the package name spelling changed then we need to update
       // it, to make sure that the subsequent commands don't fail and the
@@ -197,6 +204,8 @@ namespace bpkg
              bool replace,
              bool simulate)
   {
+    assert (session::has_current ());
+
     tracer trace ("pkg_fetch");
 
     tracer_guard tg (pdb, trace); // NOTE: sets tracer for the whole cluster.
@@ -248,10 +257,76 @@ namespace bpkg
            << "from " << pl->repository_fragment->name;
 
     auto_rmfile arm;
-    path a (pdb.config_orig / pl->location.leaf ());
+    path an (pl->location.leaf ());
+    path a (pdb.config_orig / an);
+
+    // Note that in the replace mode we first fetch the new package version
+    // archive and then update the existing selected package object, dropping
+    // the previous package version archive, if present. This way we, in
+    // particular, keep the existing selected package/archive intact if the
+    // fetch operation fails. However, this approach requires to handle
+    // re-fetching (potentially from a different repository) of the same
+    // package version specially.
+    //
+    // Specifically, if we need to overwrite the package archive file, then we
+    // stash the existing archive in the temporary directory and remove it on
+    // success. On failure, we try to move the stashed archive to the original
+    // place. Failed that either, we mark the package as broken.
+    //
+    // (If you are wondering why don't we instead always fetch into a
+    // temporary file, the answer is Windows, where moving a newly created
+    // file may not succeed because it is being scanned by Windows Defender
+    // or some such.)
+    //
+    auto_rmfile earm;
+    shared_ptr<selected_package> sp;
+
+    auto g (
+      make_exception_guard (
+        [&arm, &a, &earm, &sp, &pdb, &t] ()
+        {
+          // Restore stashed archive.
+          //
+          if (!earm.path.empty () && exists (earm.path))
+          {
+            if (mv (earm.path, a, true /* ignore_error */))
+            {
+              earm.cancel ();
+              arm.cancel ();  // Note: may not be armed yet, which is ok.
+            }
+            //
+            // Note: can already be marked as broken by pkg_purge_fs().
+            //
+            else if (sp->state != package_state::broken)
+            {
+              sp->state = package_state::broken;
+              pdb.update (sp);
+              t.commit ();
+
+              // Here we assume that mv() has already issued the diagnostics.
+              //
+              info << "package " << sp->name << pdb << " is now broken; "
+                   << "use 'pkg-purge --force' to remove";
+            }
+          }
+        }));
 
     if (!simulate)
     {
+      // Stash the existing package archive if it needs to be overwritten (see
+      // above for details).
+      //
+      // Note: compare the archive absolute paths.
+      //
+      if (replace                                          &&
+          (sp = pdb.find<selected_package> (n)) != nullptr &&
+          sp->archive                                      &&
+          sp->effective_archive (pdb.config) == pdb.config / an)
+      {
+        earm = tmp_file (pdb.config_orig, n.string () + '-' + v.string ());
+        mv (a, earm.path);
+      }
+
       pkg_fetch_archive (
         co, pl->repository_fragment->location, pl->location, a);
 
diff --git a/bpkg/pkg-fetch.hxx b/bpkg/pkg-fetch.hxx
index 5d698d5..8607178 100644
--- a/bpkg/pkg-fetch.hxx
+++ b/bpkg/pkg-fetch.hxx
@@ -37,6 +37,8 @@ namespace bpkg
   // Note that both package and repository information configurations need to
   // be passed.
   //
+  // Also note that it should be called in session.
+  //
   shared_ptr<selected_package>
   pkg_fetch (const common_options&,
              database& pdb,
diff --git a/bpkg/pkg-purge.cxx b/bpkg/pkg-purge.cxx
index 07951a6..e031b6a 100644
--- a/bpkg/pkg-purge.cxx
+++ b/bpkg/pkg-purge.cxx
@@ -58,7 +58,7 @@ namespace bpkg
         {
           if (!simulate)
           {
-            path a (p->archive->absolute () ? *p->archive : c / *p->archive);
+            path a (p->effective_archive (c));
 
             if (exists (a))
               rm (a);
@@ -192,7 +192,7 @@ namespace bpkg
 
       if (p->purge_archive)
       {
-        path a (p->archive->absolute () ? *p->archive : c / *p->archive);
+        path a (p->effective_archive (c));
 
         if (exists (a))
           fail << "archive file of broken package " << n << " still exists" <<
diff --git a/bpkg/pkg-unpack.cxx b/bpkg/pkg-unpack.cxx
index 323f296..22ff02f 100644
--- a/bpkg/pkg-unpack.cxx
+++ b/bpkg/pkg-unpack.cxx
@@ -381,7 +381,7 @@ namespace bpkg
       // If the archive path is not absolute, then it must be relative
       // to the configuration.
       //
-      path a (p->archive->absolute () ? *p->archive : c / *p->archive);
+      path a (p->effective_archive (c));
 
       l4 ([&]{trace << "archive: " << a;});
 
diff --git a/bpkg/utility.cxx b/bpkg/utility.cxx
index ec95264..d084b76 100644
--- a/bpkg/utility.cxx
+++ b/bpkg/utility.cxx
@@ -313,6 +313,30 @@ namespace bpkg
     return true;
   }
 
+  bool
+  mv (const path& from, const path& to, bool ie)
+  {
+    if (verb >= 3)
+      text << "mv " << from << ' ' << to;
+
+    try
+    {
+      mvfile (from, to,
+              cpflags::overwrite_content | cpflags::overwrite_permissions);
+    }
+    catch (const system_error& e)
+    {
+      error << "unable to move file " << from << " to " << to << ": " << e;
+
+      if (ie)
+        return false;
+
+      throw failed ();
+    }
+
+    return true;
+  }
+
   dir_path
   change_wd (const dir_path& d)
   {
diff --git a/bpkg/utility.hxx b/bpkg/utility.hxx
index f36185a..7a51948 100644
--- a/bpkg/utility.hxx
+++ b/bpkg/utility.hxx
@@ -214,6 +214,9 @@ namespace bpkg
   bool
   mv (const dir_path& from, const dir_path& to, bool ignore_errors = false);
 
+  bool
+  mv (const path& from, const path& to, bool ignore_errors = false);
+
   // Set (with diagnostics at verbosity level 3 or higher) the new and return
   // the previous working directory.
   //
diff --git a/tests/pkg-fetch.testscript b/tests/pkg-fetch.testscript
index 7d32523..5046c5d 100644
--- a/tests/pkg-fetch.testscript
+++ b/tests/pkg-fetch.testscript
@@ -160,7 +160,14 @@ $* libfoo/1.0.0 2>>/EOE != 0
     $* -e $src/t1/libfoo-1.0.0.tar.gz 2>'using libfoo/1.0.0 (external)';
     $pkg_status   libfoo/1.0.0        1>'libfoo fetched 1.0.0';
 
-    $pkg_purge libfoo 2>'purged libfoo/1.0.0'
+    $*            libfoo/1.1.0  2>'fetched libfoo/1.1.0';
+    $pkg_unpack   libfoo        2>'unpacked libfoo/1.1.0';
+    test -d   cfg/libfoo-1.1.0;
+    $*            libfoo/1.1.0  2>'fetched libfoo/1.1.0';
+    test -d   cfg/libfoo-1.1.0                             == 1;
+    $pkg_status   libfoo/1.1.0  1>'libfoo fetched 1.1.0';
+
+    $pkg_purge libfoo 2>'purged libfoo/1.1.0'
   }
 
   : purge-existing