Merge branch 'fix/dontstorediffindex' into 'main'

Do not store .diff_Index files in update

See merge request apt-team/apt!316

9 files changed, 106 insertions, 92 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 7df6483ba..400838dcb 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -276,6 +276,15 @@ static HashStringList GetExpectedHashesFromFor(metaIndex * const Parser, std::st
    return R->Hashes;
 }
 									/*}}}*/
+static void RemoveOldLeftoverDiffIndex(IndexTarget const &Target)	/*{{{*/
+{
+   std::string const FinalFile = GetFinalFileNameFromURI(GetDiffIndexURI(Target));
+   RemoveFile("TransactionCommit", FinalFile);
+   for (auto const &ext: APT::Configuration::getCompressorExtensions())
+      if (not ext.empty() && ext != ".")
+	 RemoveFile("TransactionCommit", FinalFile + ext);
+}
+									/*}}}*/
 
 class pkgAcquire::Item::Private /*{{{*/
 {
@@ -414,12 +423,16 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item)
 	 std::clog << "Skip " << Target.URI << " as transaction was already dealt with!" << std::endl;
       return false;
    }
-   std::string const FinalFile = GetFinalFilename();
-   if (TransactionManager->IMSHit == true && FileExists(FinalFile) == true)
+   if (TransactionManager->IMSHit)
    {
-      PartialFile = DestFile = FinalFile;
-      Status = StatDone;
-      return false;
+      std::string const FinalFile = GetFinalFilename();
+      if (FinalFile.empty() || FileExists(FinalFile))
+      {
+	 if (not FinalFile.empty())
+	    PartialFile = DestFile = FinalFile;
+	 Status = StatDone;
+	 return false;
+      }
    }
    // this ensures we rewrite only once and only the first step
    auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI);
@@ -508,11 +521,7 @@ std::string pkgAcquire::Item::GetFinalFilename() const
 }
 std::string pkgAcqDiffIndex::GetFinalFilename() const
 {
-   std::string const FinalFile = GetFinalFileNameFromURI(GetDiffIndexURI(Target));
-   // we don't want recompress, so lets keep whatever we got
-   if (CurrentCompressionExtension == "uncompressed")
-      return FinalFile;
-   return FinalFile + "." + CurrentCompressionExtension;
+   return {};
 }
 std::string pkgAcqIndex::GetFinalFilename() const
 {
@@ -675,12 +684,11 @@ bool pkgAcqDiffIndex::TransactionState(TransactionStates const state)
 
    switch (state)
    {
-      case TransactionStarted: _error->Fatal("Item %s changed to invalid transaction start state!", Target.URI.c_str()); break;
+      case TransactionStarted: _error->Fatal("Item %s changed to invalid transaction start state!", GetDiffIndexURI(Target).c_str()); break;
       case TransactionCommit:
+	 RemoveOldLeftoverDiffIndex(Target);
 	 break;
       case TransactionAbort:
-	 std::string const Partial = GetPartialFileNameFromURI(Target.URI);
-	 RemoveFile("TransactionAbort", Partial);
 	 break;
    }
 
@@ -761,6 +769,7 @@ class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem		/*{{{*/
 	       std::clog << "rm " << DestFile << " # " << DescURI() << std::endl;
 	    if (RemoveFile("TransItem::TransactionCommit", DestFile) == false)
 	       return false;
+	    RemoveOldLeftoverDiffIndex(Target);
 	    break;
       }
       return true;
@@ -1704,9 +1713,6 @@ void pkgAcqMetaClearSig::QueueIndexes(bool const verify)			/*{{{*/
 	 if (filename.empty() == false)
 	 {
 	    new NoActionItem(Owner, Target, filename);
-	    std::string const idxfilename = GetFinalFileNameFromURI(GetDiffIndexURI(Target));
-	    if (FileExists(idxfilename))
-	       new NoActionItem(Owner, Target, idxfilename);
 	    targetsSeen.emplace(Target.Option(IndexTarget::CREATED_BY));
 	    continue;
 	 }
@@ -2703,12 +2709,8 @@ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig cons
    new pkgAcqIndex(Owner, TransactionManager, Target);
 }
 									/*}}}*/
-bool pkgAcqDiffIndex::VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const)/*{{{*/
+bool pkgAcqDiffIndex::VerifyDone(std::string const &/*Message*/, pkgAcquire::MethodConfig const * const)/*{{{*/
 {
-   string const FinalFile = GetFinalFilename();
-   if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
-      DestFile = FinalFile;
-
    if (ParseDiffIndex(DestFile))
       return true;
 
@@ -2748,7 +2750,7 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes,	/
       }
    }
 
-   TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
+   TransactionManager->TransactionStageRemoval(this, DestFile);
 
    Complete = true;
    Status = StatDone;
diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index 727880e71..1cb55bf14 100644
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -826,11 +826,11 @@ pkgAcquire::Worker *pkgAcquire::WorkerStep(Worker *I)
    return I->NextAcquire;
 }
 									/*}}}*/
-// Acquire::CleanDir - Cleans a directory					/*{{{*/
+// CleanDir - Cleans a directory					/*{{{*/
 // ---------------------------------------------------------------------
 /* This is a bit simplistic, it looks at every file in the dir and sees
    if it matches the predicate or not. */
-bool pkgAcquire::CleanDir(string Dir, std::function<bool(char const*)> const &Keep, char const * const Caller)
+static bool CleanDir(std::string const &Dir, std::function<bool(std::string_view)> const &Keep, char const * const Caller)
 {
    // non-existing directories are by definition clean…
    if (DirectoryExists(Dir) == false)
@@ -855,7 +855,7 @@ bool pkgAcquire::CleanDir(string Dir, std::function<bool(char const*)> const &Ke
 	  strcmp(E->d_name, "lost+found") == 0 ||
 	  strcmp(E->d_name, ".") == 0 ||
 	  strcmp(E->d_name, "..") == 0 ||
-	  Keep(E->d_name) == true)
+	  Keep(E->d_name))
 	 continue;
       RemoveFileAt(Caller, dirfd, E->d_name);
    }
@@ -867,12 +867,12 @@ bool pkgAcquire::CleanDir(string Dir, std::function<bool(char const*)> const &Ke
 // ---------------------------------------------------------------------
 /* This is a bit simplistic, it looks at every file in the dir and sees
    if it is part of the download set. */
-bool pkgAcquire::Clean(string Dir)
+bool pkgAcquire::Clean(std::string Dir)
 {
    return CleanDir(
       Dir,
       // Look in the get list and if found then keep
-      [this](char const *FName) {
+      [this](std::string_view const FName) {
          return std::any_of(Items.cbegin(), Items.cend(),
             [FName](pkgAcquire::Item const * const I) {
                return flNotDir(I->DestFile) == FName;
@@ -882,21 +882,19 @@ bool pkgAcquire::Clean(string Dir)
    );
 }
                            /*}}}*/
-// Acquire::CleanLists - Cleans a directory of list files             /*{{{*/
-// ---------------------------------------------------------------------
-/* */
-bool pkgAcquire::CleanLists(string Dir)
+// Acquire::CleanLists - Cleans a directory of list files		/*{{{*/
+bool pkgAcquire::CleanLists(std::string const &Dir)
 {
+   std::regex const KeepPattern(".*_(Release|Release\\.gpg|InRelease)");
    return CleanDir(
       Dir,
-      [](char const *FName) noexcept {
-         static const std::regex KeepPattern(".*_(Release|Release.gpg|InRelease)");
-         return std::regex_match(FName, KeepPattern);
+      [&KeepPattern](std::string_view const FName) noexcept {
+         return std::regex_match(FName.begin(), FName.end(), KeepPattern);
       },
       "pkgAcquire::CleanLists"
    );
 }
-                           /*}}}*/
+									/*}}}*/
 // Acquire::TotalNeeded - Number of bytes to fetch			/*{{{*/
 // ---------------------------------------------------------------------
 /* This is the total number of bytes needed */
diff --git a/apt-pkg/acquire.h b/apt-pkg/acquire.h
index a5f7a40c8..f7c40aa5f 100644
--- a/apt-pkg/acquire.h
+++ b/apt-pkg/acquire.h
@@ -70,7 +70,6 @@
 #include <apt-pkg/weakptr.h>
 
 #include <chrono>
-#include <functional>
 #include <string>
 #include <vector>
 
@@ -338,7 +337,7 @@ class APT_PUBLIC pkgAcquire
     *
     *  \return \b true if the directory exists and is readable.
     */
-   bool CleanLists(std::string Dir);
+   bool CleanLists(std::string const &Dir);
 
    /** \return the total size in bytes of all the items included in
     *  this download.
@@ -381,20 +380,6 @@ class APT_PUBLIC pkgAcquire
 
    private:
    APT_HIDDEN void Initialize();
-
-   /** Delete each entry in the given directory whose name does \em not match
-    *  a criterion.
-    *
-    *  \param Dir The directory to be cleaned.
-    *
-    *  \param KeepP A predicate telling to keep the named file; if it is
-    *  non-empty and returns true, the file is kept.
-    *
-    *  \param Caller Log name of the caller.
-    *
-    *  \return \b true if the directory exists and is readable.
-    */
-   APT_HIDDEN static bool CleanDir(std::string Dir, std::function<bool(char const*)> const &KeepP, char const * const Caller);
 };
 
 /** \brief Represents a single download source from which an item
diff --git a/apt-private/private-cmndline.cc b/apt-private/private-cmndline.cc
index 980d00530..9e3938407 100644
--- a/apt-private/private-cmndline.cc
+++ b/apt-private/private-cmndline.cc
@@ -242,14 +242,15 @@ static bool addArgumentsAPTGet(std::vector<CommandLine::Args> &Args, char const
       addArg(0,"format","APT::Get::IndexTargets::Format", CommandLine::HasArg);
       addArg(0,"release-info","APT::Get::IndexTargets::ReleaseInfo", 0);
    }
-   else if (CmdMatches("clean", "autoclean", "auto-clean", "check", "download", "changelog") ||
+   else if (CmdMatches("clean", "autoclean", "auto-clean", "distclean", "dist-clean", "check", "download", "changelog") ||
 	    CmdMatches("markauto", "unmarkauto")) // deprecated commands
       ;
    else if (CmdMatches("moo"))
       addArg(0, "color", "APT::Moo::Color", 0);
 
    if (CmdMatches("install", "reinstall", "remove", "purge", "upgrade", "dist-upgrade",
-	    "dselect-upgrade", "autoremove", "auto-remove", "autopurge", "clean", "autoclean", "auto-clean", "check",
+	    "dselect-upgrade", "autoremove", "auto-remove", "autopurge", "check",
+	    "clean", "autoclean", "auto-clean", "distclean", "dist-clean",
 	    "build-dep", "satisfy", "full-upgrade", "source"))
    {
       addArg('s', "simulate", "APT::Get::Simulate", 0);
diff --git a/apt-private/private-download.cc b/apt-private/private-download.cc
index b271ec03f..84a088f3d 100644
--- a/apt-private/private-download.cc
+++ b/apt-private/private-download.cc
@@ -303,20 +303,8 @@ bool DoChangelog(CommandLine &CmdL)
    return true;
 }
 									/*}}}*/
-
-// DoClean - Remove download archives            /*{{{*/
-bool DoClean(CommandLine &)
-{
-   return DoDistClean(false);
-}
-                           /*}}}*/
-// DoDistClean - Remove download archives and lists            /*{{{*/
-bool DoDistClean(CommandLine &)
-{
-   return DoDistClean(true);
-}
-// DoDistClean - the worker
-bool DoDistClean(bool ListsToo)
+// DoClean & DoDistClean - Remove download archives and/or lists	/*{{{*/
+static bool CleanDownloadDirectories(bool const ListsToo)
 {
    std::string const archivedir = _config->FindDir("Dir::Cache::archives");
    std::string const listsdir = _config->FindDir("Dir::state::lists");
@@ -325,22 +313,24 @@ bool DoDistClean(bool ListsToo)
    {
       std::string const pkgcache = _config->FindFile("Dir::cache::pkgcache");
       std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache");
-      std::cout << "Del " << archivedir << "* " << archivedir << "partial/*"<< std::endl
-      << "Del " << listsdir << "partial/*" << (ListsToo ? " *_{Packages,Sources}{,.diff_Index}" : "") << std::endl
-      << "Del " << pkgcache << " " << srcpkgcache << std::endl;
+      std::cout << "Del " << archivedir << "* " << archivedir << "partial/*" << std::endl
+		<< "Del " << listsdir << "partial/*" << std::endl;
+      if (ListsToo)
+	 std::cout << "Del " << listsdir << "*_{Packages,Sources,Translation-*}" << std::endl;
+      std::cout << "Del " << pkgcache << " " << srcpkgcache << std::endl;
       return true;
    }
 
    pkgAcquire Fetcher;
-   if (archivedir.empty() == false && FileExists(archivedir) == true &&
-    Fetcher.GetLock(archivedir) == true)
+   if (not archivedir.empty() && FileExists(archivedir) &&
+       Fetcher.GetLock(archivedir))
    {
       Fetcher.Clean(archivedir);
       Fetcher.Clean(archivedir + "partial/");
    }
 
-   if (listsdir.empty() == false && FileExists(listsdir) == true &&
-    Fetcher.GetLock(listsdir) == true)
+   if (not listsdir.empty() && FileExists(listsdir) &&
+       Fetcher.GetLock(listsdir))
    {
       Fetcher.Clean(listsdir + "partial/");
       if (ListsToo)
@@ -351,6 +341,14 @@ bool DoDistClean(bool ListsToo)
 
    return true;
 }
+bool DoClean(CommandLine &)
+{
+   return CleanDownloadDirectories(false);
+}
+bool DoDistClean(CommandLine &)
+{
+   return CleanDownloadDirectories(true);
+}
 									/*}}}*/
 // DoAutoClean - Smartly remove downloaded archives			/*{{{*/
 // ---------------------------------------------------------------------
diff --git a/apt-private/private-download.h b/apt-private/private-download.h
index 918828241..e13ade1c6 100644
--- a/apt-private/private-download.h
+++ b/apt-private/private-download.h
@@ -34,7 +34,6 @@ APT_PUBLIC bool DoChangelog(CommandLine &CmdL);
 
 APT_PUBLIC bool DoClean(CommandLine &CmdL);
 APT_PUBLIC bool DoDistClean(CommandLine &CmdL);
-bool DoDistClean(bool ListsToo);
 APT_PUBLIC bool DoAutoClean(CommandLine &CmdL);
 
 #endif
diff --git a/cmdline/apt-get.cc b/cmdline/apt-get.cc
index 95c4a85ab..d41e0f243 100644
--- a/cmdline/apt-get.cc
+++ b/cmdline/apt-get.cc
@@ -424,6 +424,8 @@ static std::vector<aptDispatchWithHelp> GetCommands()			/*{{{*/
       {"clean", &DoClean, _("Erase downloaded archive files")},
       {"autoclean", &DoAutoClean, _("Erase old downloaded archive files")},
       {"auto-clean", &DoAutoClean, nullptr},
+      {"distclean", &DoDistClean, nullptr},
+      {"dist-clean", &DoDistClean, nullptr},
       {"check", &DoCheck, _("Verify that there are no broken dependencies")},
       {"source", &DoSource, _("Download source archives")},
       {"download", &DoDownload, _("Download the binary package into the current directory")},
diff --git a/test/integration/test-apt-get-clean b/test/integration/test-apt-get-clean
index d05073218..32dd3e852 100755
--- a/test/integration/test-apt-get-clean
+++ b/test/integration/test-apt-get-clean
@@ -13,13 +13,27 @@ insertpackage 'experimental' 'foo' 'all' '1:1'
 insertinstalledpackage 'foo' 'all' '3'
 
 setupaptarchive --no-update
+mkdir -p rootdir/var/lib/apt/lists/partial
+testsuccess test -d rootdir/var/lib/apt/lists/partial
+
+# nothing to do always works (part 1)
+for cmd in aptget apt; do
+	for sub in distclean dist-clean; do
+		testsuccess $cmd $sub
+		testsuccess $cmd $sub -s
+	done
+done
+testsuccess test -d rootdir/var/lib/apt/lists/partial
 
-mkdir -p rootdir/var/lib/apt/lists/lost+found
 testsuccess apt update
 
-# nothing to do always works
-testsuccess aptget clean
-testsuccess aptget clean -s
+# nothing to do always works (part 2)
+for cmd in aptget apt; do
+	for sub in clean autoclean auto-clean; do
+		testsuccess $cmd $sub
+		testsuccess $cmd $sub -s
+	done
+done
 
 # generate some dirt and clean it up
 generatedirt() {
@@ -63,6 +77,23 @@ if [ "$(id -u)" != '0' ]; then
 	chmod 644 rootdir/var/cache/apt/archives/lock
 fi
 
+msgmsg 'distclean'
+listcurrentlistsdirectory > aptdistclean.before
+testsuccessequal '3' grep -c -e 'Release$' aptdistclean.before
+testsuccess grep -e 'Packages$' -e 'Sources$' -e 'Translation-*$' aptdistclean.before
+testsuccess apt distclean
+listcurrentlistsdirectory > aptdistclean.after
+testfailure cmp aptdistclean.before aptdistclean.after
+testsuccessequal '3' grep -c -e 'Release$' aptdistclean.after
+testfailureequal '0' grep -c -e 'Packages$' -e 'Sources$' -e 'Translation-*$' aptdistclean.after
+for dirt in Packages.gz Sources.xz Release2gpg Release_Packages; do
+	touch "rootdir/var/lib/apt/lists/_foo_bar_$dirt"
+done
+listcurrentlistsdirectory > aptdistclean.before
+testsuccess apt distclean
+listcurrentlistsdirectory > aptdistclean.cleaned
+testsuccess cmp aptdistclean.after aptdistclean.cleaned
+
 directorygone() {
 	rm -rf "$1"
 	testsuccess apt autoclean
@@ -86,3 +117,4 @@ directorygone 'rootdir/var/lib/apt/lists'
 msgmsg 'apt directory missing'
 directorygone 'rootdir/var/cache/apt'
 directorygone 'rootdir/var/lib/apt'
+
diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 38e455b00..cf3a2f2d9 100755
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -38,14 +38,7 @@ wasmergeused() {
 	#apt update "$@" 2>&1 | tee rootdir/tmp/testsuccess.output
 
 	msgtest 'No intermediate patch files' 'still exist'
-	local EDS="$(find rootdir/var/lib/apt/lists -name '*.ed' -o -name '*.ed.*')"
-	if [ -z "$EDS" ]; then
-		msgpass
-	else
-		echo
-		echo "$EDS"
-		msgfail
-	fi
+	testempty --nomsg find rootdir/var/lib/apt/lists -name '*.ed' -o -name '*.ed.*' -o -name '*.diff_Index' -o -name '*.diff_Index.xz'
 
 	if echo "$*" | grep -q -- '-o test::cannot-use-pdiff=1'; then
 		msgtest 'Check if pdiff was' 'not used'
@@ -389,21 +382,25 @@ testcase() {
 	testrun -o Acquire::PDiffs::Merge=1 -o APT::Get::List-Cleanup=0 "$@"
 }
 generatepartialleftovers() {
+	local PREFIX="$1"
+	shift
 	for f in "$@"; do
-		cat "${PKGFILE}" "${PKGFILE}" > "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${f}"
-		printf '\n\nInvalid\nStanza: yes\n\n' >> "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${f}"
+		cat "${PKGFILE}" "${PKGFILE}" > "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${PREFIX}${f}"
+		printf '\n\nInvalid\nStanza: yes\n\n' >> "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${PREFIX}${f}"
 	done
+	printf '\n\nInvalid\nStanza: yes\n\n' >> "rootdir/var/lib/apt/lists-bak/localhost:${APTHTTPPORT}_${PREFIX}Packages.diff_Index"
+	printf '\n\nInvalid\nStanza: yes\n\n' >> "rootdir/var/lib/apt/lists-bak/localhost:${APTHTTPPORT}_${PREFIX}Packages.diff_Index.xz"
 }
 
-partialleftovers() { generatepartialleftovers 'Packages' 'Packages-patched'; }
+partialleftovers() { generatepartialleftovers '' 'Packages' 'Packages-patched'; }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; testsuccess test -e "rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_Packages"; }
 testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=false
-partialleftovers() { generatepartialleftovers "Packages.${LOWCOSTEXT}" "Packages-patched.${LOWCOSTEXT}"; }
+partialleftovers() { generatepartialleftovers '' "Packages.${LOWCOSTEXT}" "Packages-patched.${LOWCOSTEXT}"; }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; testsuccess test -e "rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_Packages.$LOWCOSTEXT"; }
 testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=true
 
 
-partialleftovers() { generatepartialleftovers "redirectme_Packages.${LOWCOSTEXT}" "redirectme_Packages-patched.${LOWCOSTEXT}"; }
+partialleftovers() { generatepartialleftovers 'redirectme_' "Packages.${LOWCOSTEXT}" "Packages-patched.${LOWCOSTEXT}"; }
 # redirect the InRelease file only – the other files are auto-redirected by apt
 webserverconfig 'aptwebserver::redirect::replace::/redirectme/I' "http://0.0.0.0:${APTHTTPPORT}/I"
 rewritesourceslist "http://localhost:${APTHTTPPORT}/redirectme"