From c82f96210eb62c92d31ded7073f4cf5371cc9485 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Jan 2024 13:53:09 +0000 Subject: Improve and test distclean implementation The implementation as-is as various smaller/esoteric bugs and inconsistencies like apt-get not supporting them, the option -s being supported in code but not accepted on the command line, the regex not escaping the dot before the file extension and exposing more implementation details to public headers than we actually need. Also comes with a small test case to ensure it actually works. References: bd7c126e3fb1b94e76e0e632c657cea854586844 --- apt-pkg/acquire.cc | 24 +++++++++++------------- apt-pkg/acquire.h | 17 +---------------- 2 files changed, 12 insertions(+), 29 deletions(-) (limited to 'apt-pkg') 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 const &Keep, char const * const Caller) +static bool CleanDir(std::string const &Dir, std::function 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 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 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 #include -#include #include #include @@ -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 const &KeepP, char const * const Caller); }; /** \brief Represents a single download source from which an item -- cgit v1.2.3-70-g09d2 From afcdbcf895284efd76903b2b3ba5cc849059ce50 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Jan 2024 16:25:35 +0000 Subject: Do not store .diff_Index files in update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nowadays we only download the index file if we have a non-current file on disk which we want to patch. If that is the case, any index file for patches we could have stored is by definition outdated, so storing those files just takes up disk space. At least, that is the case if we have a Release file – if we don't this commit introduces a needless redownload for such repositories but such repositories are an error by default and if they can't be bothered to provide a Release file its very unlikely they actually ship diffs, so adding detection code for this seems pointless at best. --- apt-pkg/acquire-item.cc | 46 ++++++++++++++++++++------------------- apt-private/private-download.cc | 2 +- test/integration/test-pdiff-usage | 23 +++++++++----------- 3 files changed, 35 insertions(+), 36 deletions(-) (limited to 'apt-pkg') 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-private/private-download.cc b/apt-private/private-download.cc index d95c8ef43..84a088f3d 100644 --- a/apt-private/private-download.cc +++ b/apt-private/private-download.cc @@ -316,7 +316,7 @@ static bool CleanDownloadDirectories(bool const ListsToo) std::cout << "Del " << archivedir << "* " << archivedir << "partial/*" << std::endl << "Del " << listsdir << "partial/*" << std::endl; if (ListsToo) - std::cout << "Del " << listsdir << "*_{Packages,Sources,Translation-*}{,.diff_Index}" << std::endl; + std::cout << "Del " << listsdir << "*_{Packages,Sources,Translation-*}" << std::endl; std::cout << "Del " << pkgcache << " " << srcpkgcache << std::endl; return true; } 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" -- cgit v1.2.3-70-g09d2