Delete stale cache files on update

This actually addresses #66, though not in the way that I originall
expected. Instead of forcing pkgfile to parse through pacman.conf on
every search/list invocation, cleanup bad cache entries when performing
an update. This way we get eventual consistency with pacman.conf.

Author: falconindy

meson.build

@@ -180,12 +180,13 @@ if gtest.found() and gmock.found()
     gtest_main = static_library('gtest_main', 'src/test/gtest_main.cc')
 
     test(
-        'filter_test',
+        'unit_tests',
         executable(
-            'filter_test',
+            'unit_tests',
             files(
                 '''
         src/filter_test.cc
+        src/repo_test.cc
       '''.split(),
             ),
             link_with: [libcommon, gtest_main],

src/archive_converter.cc

@@ -137,7 +137,8 @@ bool ArchiveConverter::Finalize() {
   for (int i = chunk_number_ + 1;; ++i) {
     std::string path = MakeArchiveChunkFilename(base_filename_out_, i, false);
 
-    if (unlink(path.c_str()) != 0) {
+    std::error_code ec;
+    if (!fs::remove(path, ec)) {
       break;
     }
   }

src/pkgfile.cc

@@ -16,6 +16,7 @@
 #include "compress.hh"
 #include "filter.hh"
 #include "queue.hh"
+#include "repo.hh"
 #include "result.hh"
 #include "update.hh"
 
@@ -323,28 +324,6 @@ std::unique_ptr<filter::Filter> Pkgfile::BuildFilterFromOptions(
   return filter;
 }
 
-// Repo files should always be of the form ${reponame}.files.nnn where 'n' is an
-// 0-indexed, zero-padded, increasing integer
-bool HasRepoSuffix(std::string_view path) {
-  const auto ndots = std::count(path.begin(), path.end(), '.');
-  if (ndots != 2) {
-    return false;
-  }
-
-  auto pos = path.rfind('.');
-  if (!path.substr(0, pos).ends_with(".files")) {
-    return false;
-  }
-
-  for (++pos; pos < path.size(); ++pos) {
-    if (!isdigit(path[pos])) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 // static
 Pkgfile::RepoMap Pkgfile::DiscoverRepos(std::string_view cachedir,
                                         std::error_code& ec) {
@@ -355,7 +334,7 @@ Pkgfile::RepoMap Pkgfile::DiscoverRepos(std::string_view cachedir,
   // they're lexicographically ordered.
   for (const auto& entry : fs::directory_iterator(cachedir, ec)) {
     const std::string pathname = entry.path();
-    if (!entry.is_regular_file() || !HasRepoSuffix(pathname)) {
+    if (!entry.is_regular_file() || !FilenameHasRepoSuffix(pathname)) {
       continue;
     }
 

src/repo.cc

@@ -166,4 +166,24 @@ Repo::~Repo() {
   }
 }
 
+bool FilenameHasRepoSuffix(std::string_view path) {
+  const auto ndots = std::count(path.begin(), path.end(), '.');
+  if (ndots != 2) {
+    return false;
+  }
+
+  auto pos = path.rfind('.');
+  if (!path.substr(0, pos).ends_with(".files")) {
+    return false;
+  }
+
+  for (++pos; pos < path.size(); ++pos) {
+    if (!isdigit(path[pos])) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 // vim: set ts=2 sw=2 et:

src/repo.hh

@@ -61,4 +61,8 @@ struct AlpmConfig {
   std::string architecture;
 };
 
+// Verifies that the given file path has a format of "${reponame}.files.nnn"
+// where 'n' is an 0-indexed, zero-padded, increasing integer.
+bool FilenameHasRepoSuffix(std::string_view path);
+
 // vim: set ts=2 sw=2 et:

src/update.cc

@@ -10,6 +10,7 @@
 #include <sys/utsname.h>
 
 #include <algorithm>
+#include <filesystem>
 #include <format>
 
 #include "archive_converter.hh"
@@ -18,6 +19,7 @@
 #include "repo.hh"
 
 namespace chrono = std::chrono;
+namespace fs = std::filesystem;
 
 auto now = chrono::system_clock::now;
 
@@ -254,6 +256,24 @@ bool Updater::RepackRepoData(const struct Repo* repo) {
   return converter != nullptr && converter->RewriteArchive();
 }
 
+void Updater::TidyCacheDir(const std::set<std::string>& known_repos) {
+  std::error_code ec;
+  for (const auto& entry : fs::directory_iterator(cachedir_, ec)) {
+    const fs::path filename = entry.path().filename();
+    const fs::path reponame = entry.path().stem().stem();
+
+    if (!FilenameHasRepoSuffix(filename.c_str()) ||
+        !known_repos.contains(reponame)) {
+      std::error_code ec;
+      fs::remove(entry, ec);
+      if (ec.value() != 0) {
+        fprintf(stderr, "warning: failed to remove stale cache file: %s\n",
+                entry.path().c_str());
+      }
+    }
+  }
+}
+
 void Updater::DownloadWaitLoop(CURLM* multi) {
   int active_handles;
 
@@ -417,6 +437,13 @@ int Updater::Update(const std::string& alpm_config_file, bool force) {
     ret = 1;
   }
 
+  std::set<std::string> known_repos;
+  for (const auto& repo : alpm_config.repos) {
+    known_repos.insert(repo.name);
+  }
+
+  TidyCacheDir(known_repos);
+
   return ret;
 }
 

src/update.hh

@@ -2,6 +2,8 @@
 
 #include <curl/curl.h>
 
+#include <set>
+
 #include "pkgfile.hh"
 #include "repo.hh"
 
@@ -19,6 +21,7 @@ class Updater {
   void DownloadWaitLoop(CURLM* multi);
   int DownloadCheckComplete(CURLM* multi, int remaining);
   bool RepackRepoData(const struct Repo* repo);
+  void TidyCacheDir(const std::set<std::string>& known_repos);
 
   std::string cachedir_;
   int compress_;

tests/pkgfile_test.py

@@ -8,6 +8,7 @@
 import tempfile
 import time
 import unittest
+from pathlib import Path
 
 
 def FindMesonBuildDir():
@@ -46,7 +47,7 @@ class TestCase(unittest.TestCase):
     def setUp(self):
         self.build_dir = FindMesonBuildDir()
         self._tempdir = tempfile.TemporaryDirectory()
-        self.tempdir = self._tempdir.name
+        self.tempdir = Path(self._tempdir.name)
         self.cachedir = os.path.join(
             os.path.dirname(os.path.realpath(__file__)), 'golden/pkgfile')
         self.alpmcachedir = os.path.join(

tests/update.py

@@ -5,6 +5,7 @@
 import pkgfile_test
 import os
 from collections import defaultdict
+from pathlib import Path
 
 
 def _sha256(path):
@@ -23,12 +24,12 @@ def setUp(self):
 
         # relocate our cachedir to not overwrite the golden fileset
         self.goldendir = self.cachedir
-        self.cachedir = os.path.join(self.tempdir, 'cache')
-        os.mkdir(self.cachedir)
+        self.cachedir = Path(self.tempdir, 'cache')
+        self.cachedir.mkdir()
 
     @staticmethod
     def getRepoFiles(subdir, reponame):
-        return sorted(glob.glob('{}/{}.files.*'.format(subdir, reponame)))
+        return sorted(Path(subdir).glob(f'{reponame}.files.*'))
 
     def assertMatchesGolden(self, reponame):
         golden_files = self.getRepoFiles(self.goldendir, reponame)
@@ -48,15 +49,14 @@ def testUpdate(self):
         self.assertMatchesGolden('testing')
 
         for repo in ('multilib', 'testing'):
-            original_repo = '{}/x86_64/{repo}/{repo}.files'.format(
-                self.alpmcachedir, repo=repo)
+            original_repo = Path(self.alpmcachedir, 'x86_64', repo, f'{repo}.files')
 
             for converted_repo in self.getRepoFiles(self.cachedir, repo):
                 # Only compare the integer portion of the mtime. we'll only ever
                 # get back second precision from a remote server, so any fractional
                 # second that's present on our golden repo can be ignored.
-                self.assertEqual(int(os.stat(original_repo).st_mtime),
-                                 int(os.stat(converted_repo).st_mtime))
+                self.assertEqual(int(original_repo.stat().st_mtime),
+                                 int(converted_repo.stat().st_mtime))
 
     def testUpdateForcesUpdates(self):
         r = self.Pkgfile(['-u'])
@@ -65,17 +65,15 @@ def testUpdateForcesUpdates(self):
         inodes_before = {}
         for r in ('multilib', 'testing'):
             for repofile in self.getRepoFiles(self.cachedir, r):
-                inodes_before[os.path.basename(repofile)] = os.stat(
-                    repofile).st_ino
+                inodes_before[repofile.name] = repofile.stat().st_ino
 
         r = self.Pkgfile(['-uu'])
         self.assertEqual(r.returncode, 0)
 
         inodes_after = {}
         for r in ('multilib', 'testing'):
             for repofile in self.getRepoFiles(self.cachedir, r):
-                inodes_after[os.path.basename(repofile)] = os.stat(
-                    repofile).st_ino
+                inodes_after[repofile.name] = repofile.stat().st_ino
 
         for r in ('multilib', 'testing'):
             self.assertNotEqual(
@@ -87,25 +85,25 @@ def testUpdateSkipsUpToDate(self):
         r = self.Pkgfile(['-u'])
         self.assertEqual(r.returncode, 0)
 
-        # gather mtimes
+        # gather inodes before the update
         inodes_before = defaultdict(dict)
         for r in ('multilib', 'testing'):
             for repofile in self.getRepoFiles(self.cachedir, r):
-                inodes_before[r][repofile] = os.stat(repofile).st_ino
+                inodes_before[r][repofile] = repofile.stat().st_ino
 
         # set the mtime to the epoch, expect that it gets rewritten on next update
-        os.utime(os.path.join(self.cachedir, 'testing.files.000'), (0, 0))
+        os.utime(self.cachedir / 'testing.files.000', (0, 0))
 
         r = self.Pkgfile(['-u'])
         self.assertEqual(r.returncode, 0)
 
-        # re-gather mtimes after a soft update
+        # re-gather inodes after a soft update
         inodes_after = defaultdict(dict)
         for r in ('multilib', 'testing'):
             for repofile in self.getRepoFiles(self.cachedir, r):
-                inodes_after[r][repofile] = os.stat(repofile).st_ino
+                inodes_after[r][repofile] = repofile.stat().st_ino
 
-        # compare to mtimes after
+        # compare inodes
         self.assertEqual(
             inodes_before['multilib'],
             inodes_after['multilib'],
@@ -117,53 +115,64 @@ def testUpdateSkipsUpToDate(self):
             msg='testing.files unexpectedly NOT rewritten by `pkgfile -u`')
 
     def testUpdateSkipsBadServer(self):
-        with open(os.path.join(self.tempdir, 'pacman.conf'), 'w') as f:
-            f.write('''
+        Path(self.tempdir / 'pacman.conf').write_text(f'''
             [options]
             Architecture = x86_64
 
             [testing]
-            Server = {fakehttp_server}/$arch/$repo/404
-            Server = {fakehttp_server}/$arch/$repo
+            Server = {self.baseurl}/$arch/$repo/404
+            Server = {self.baseurl}/$arch/$repo
 
             [multilib]
-            Server = {fakehttp_server}/$arch/$repo
-            '''.format(fakehttp_server=self.baseurl))
+            Server = {self.baseurl}/$arch/$repo
+            ''')
 
         r = self.Pkgfile(['-u'])
         self.assertEqual(r.returncode, 0)
 
     def testUpdateFailsWhenExhaustingServers(self):
-        with open(os.path.join(self.tempdir, 'pacman.conf'), 'w') as f:
-            f.write('''
+        Path(self.tempdir, 'pacman.conf').write_text(f'''
             [options]
             Architecture = x86_64
 
             [testing]
-            Server = {fakehttp_server}/$arch/$repo/404
+            Server = {self.baseurl}/$arch/$repo/404
 
             [multilib]
-            Server = {fakehttp_server}/$arch/$repo
-            '''.format(fakehttp_server=self.baseurl))
+            Server = {self.baseurl}/$arch/$repo
+            ''')
 
         r = self.Pkgfile(['-u'])
         self.assertNotEqual(r.returncode, 0)
 
     def testUpdateCleansUpOldRepoChunks(self):
         r = self.Pkgfile(['-uu', '--repochunkbytes=5000'])
         self.assertEqual(r.returncode, 0)
-        small_chunks = set(glob.glob('{}/testing.files*'.format(
-            self.cachedir)))
+        small_chunks = set(Path(self.cachedir).glob('testing.files*'))
 
         # update again, creating ~half as many repo files
         r = self.Pkgfile(['-uu', '--repochunkbytes=200000'])
         self.assertEqual(r.returncode, 0)
-        large_chunks = set(glob.glob('{}/testing.files*'.format(
-            self.cachedir)))
+        large_chunks = set(Path(self.cachedir).glob('testing.files*'))
 
         # the 100k chunked fileset is a strict subset of the original 5k chunked fileset.
         self.assertLess(large_chunks, small_chunks)
 
+    def testUpdateRemovesUnknownRepos(self):
+        expected_removed = (
+            self.cachedir / "garbage.files",
+            self.cachedir / "deletemebro.files.000",
+        )
+
+        for p in expected_removed:
+            p.touch()
+
+        r = self.Pkgfile(['-u'])
+        self.assertEqual(r.returncode, 0)
+
+        for p in expected_removed:
+            self.assertFalse(p.exists(), msg='{p} still exists, expected deleted')
+
 
 if __name__ == '__main__':
     pkgfile_test.main()