From 246f66561e23911b9615bd337b3b6f6f25b6cd31 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 6 Mar 2021 19:55:09 +0100 Subject: Rename pdiff merge patches only after they are all downloaded The rred method expects the patches to have a certain name, which we have to rename the file to before calling the method, but by delaying the rename we ensure that if the download of one of them fails and a successful fallback occurs they are all properly cleaned up as no longer useful while in the error case the next apt run can potentially pick them up as already downloaded. Our test-pdiff-usage test was encountering this every other run, but did not fail as the check for unaccounted files in partial/ was wrapped in a subshell so that the failure produced failing output, but did not change the exit code. --- test/integration/test-pdiff-usage | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'test/integration/test-pdiff-usage') diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index c5726dd08..5844619b8 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -254,7 +254,7 @@ SHA256-Download: # This should work in at least 4% of the cases... for i in $(seq 25); do testfailure apt update "$@" - if ! grep 'rred:600' rootdir/tmp/testfailure.output; then + if ! grep -q 'rred:600' rootdir/tmp/testfailure.output; then break fi done @@ -281,7 +281,8 @@ SHA256-Download: rm "${PATCHFILE}.gz" testsuccess apt update "$@" cp rootdir/tmp/testsuccess.output patchdownload.output - testsuccess grep '^Falling back to normal index file acquire' patchdownload.output + # it should be anchored on line start, but multiple processes on the same output stream… + testsuccess grep 'Falling back to normal index file acquire' patchdownload.output testnopackage oldstuff testsuccessequal "$(cat Packages-future) " aptcache show apt newstuff futurestuff -- cgit v1.2.3-70-g09d2 From 59933938f51105066161a6eb88253006826336a2 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 7 Mar 2021 00:47:26 +0100 Subject: Start pdiff patching from the last possible starting point Especially in small sections of an archive it can happen that an index returns to a previous state (e.g. if a package was first added and then removed with no other changes happening in between). The result is that we have multiple patches which start from the same hash which if we perform clientside merging is no problem although not ideal as we perform needless work. For serverside merging it would not matter, but due to rred previously refusing to merge zero-size patches but dak ignoring failure letting it carry these size-zero patches until they naturally expire we run into a problem as these broken patches won't do and force us to fall back to downloading the entire index. By always starting from the last patch instead of the first with the starter hash we can avoid this problem and behave optimally in clientside merge cases, too. --- apt-pkg/acquire-item.cc | 26 ++++++++------------------ test/integration/test-pdiff-usage | 3 +++ 2 files changed, 11 insertions(+), 18 deletions(-) (limited to 'test/integration/test-pdiff-usage') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 6dc424426..cc3d2f1ff 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -2564,26 +2564,16 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/ } } - - bool foundStart = false; - for (std::vector::iterator cur = available_patches.begin(); - cur != available_patches.end(); ++cur) - { - if (LocalHashes != cur->result_hashes) - continue; - - available_patches.erase(available_patches.begin(), cur); - foundStart = true; - break; - } - - if (foundStart == false || unlikely(available_patches.empty() == true)) { - ErrorText = "Couldn't find the start of the patch series"; - return false; - } + auto const foundStart = std::find_if(available_patches.rbegin(), available_patches.rend(), + [&](auto const &cur) { return LocalHashes == cur.result_hashes; }); + if (foundStart == available_patches.rend() || unlikely(available_patches.empty())) + { + ErrorText = "Couldn't find the start of the patch series"; + return false; + } + available_patches.erase(available_patches.begin(), std::prev(foundStart.base())); - { auto const patch = std::find_if(available_patches.cbegin(), available_patches.cend(), [](auto const &patch) { return not patch.result_hashes.usable() || not patch.patch_hashes.usable() || diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index 5844619b8..b727aa138 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -100,12 +100,15 @@ testrun() { PATCHINDEX='aptarchive/Packages.diff/Index' echo "SHA256-Current: $(sha256sum "${PKGFILE}-new" | cut -d' ' -f 1) $(stat -c%s "${PKGFILE}-new") SHA256-History: + $(sha256sum "$PKGFILE" | cut -d' ' -f 1) $(stat -c%s "$PKGFILE") 2000-08-18-2013.28 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b 33053002 2010-08-18-2013.28 $(sha256sum "$PKGFILE" | cut -d' ' -f 1) $(stat -c%s "$PKGFILE") $(basename "$PATCHFILE") SHA256-Patches: + $(sha256sum "$PATCHFILE" | cut -d' ' -f 1) $(stat -c%s "$PATCHFILE") 2000-08-18-2013.28 e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 19722 2010-08-18-2013.28 $(sha256sum "$PATCHFILE" | cut -d' ' -f 1) $(stat -c%s "$PATCHFILE") $(basename "$PATCHFILE") SHA256-Download: + $(sha256sum "${PATCHFILE}.gz" | cut -d' ' -f 1) $(stat -c%s "${PATCHFILE}.gz") 2000-08-18-2013.28.gz d2a1b33187ed2d248eeae3b1223ea71791ea35f2138a713ed371332a6421f467 197 2010-08-18-2013.28.gz $(sha256sum "${PATCHFILE}.gz" | cut -d' ' -f 1) $(stat -c%s "${PATCHFILE}.gz") $(basename "${PATCHFILE}.gz")" > "$PATCHINDEX" -- cgit v1.2.3-70-g09d2