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.

3 files changed, 13 insertions, 13 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index c9e81070b..6dc424426 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -3050,14 +3050,11 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
       State = StateErrorDiff;
       return;
    }
-   std::string const PatchFile = GetMergeDiffsPatchFileName(UnpatchedFile, patch.file);
    std::string const PatchedFile = GetKeepCompressedFileName(UncompressedUnpatchedFile, Target);
 
    switch (State)
    {
       case StateFetchDiff:
-	 Rename(DestFile, PatchFile);
-
 	 // check if this is the last completed diff
 	 State = StateDoneDiff;
 	 for (std::vector<pkgAcqIndexMergeDiffs *>::const_iterator I = allPatches->begin();
@@ -3068,6 +3065,8 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 		  std::clog << "Not the last done diff in the batch: " << Desc.URI << std::endl;
 	       return;
 	    }
+	 for (auto * diff : *allPatches)
+	    Rename(diff->DestFile, GetMergeDiffsPatchFileName(UnpatchedFile, diff->patch.file));
 	 // this is the last completed diff, so we are ready to apply now
 	 DestFile = GetKeepCompressedFileName(UncompressedUnpatchedFile + "-patched", Target);
 	 if(Debug)
@@ -3098,8 +3097,8 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 	 if(Debug)
 	    std::clog << "allDone: " << DestFile << "\n" << std::endl;
 	 return;
-      case StateDoneDiff: _error->Fatal("Done called for %s which is in an invalid Done state", PatchFile.c_str()); break;
-      case StateErrorDiff: _error->Fatal("Done called for %s which is in an invalid Error state", PatchFile.c_str()); break;
+      case StateDoneDiff: _error->Fatal("Done called for %s which is in an invalid Done state", patch.file.c_str()); break;
+      case StateErrorDiff: _error->Fatal("Done called for %s which is in an invalid Error state", patch.file.c_str()); break;
    }
 }
 									/*}}}*/
diff --git a/test/integration/framework b/test/integration/framework
index 696fcd8cd..cfde80329 100644
--- a/test/integration/framework
+++ b/test/integration/framework
@@ -2126,6 +2126,9 @@ aptautotest() {
 	fi
 }
 
+cdfind() {
+	( cd /; find "$@" )
+}
 aptautotest_aptget_update() {
 	local TESTCALL="$1"
 	while [ -n "$2" ]; do
@@ -2135,24 +2138,21 @@ aptautotest_aptget_update() {
 	if ! test -d "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists"; then return; fi
 	testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755"
 	testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755"
-	(
-	cd /
 	# all copied files are properly chmodded
 	local backupIFS="$IFS"
 	IFS="$(printf "\n\b")"
-	find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -type f ! -name 'lock' | while read file; do
+	cdfind "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -type f ! -name 'lock' | while read file; do
 		testfilestats "$file" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:644"
 	done
 	IFS="$backupIFS"
 	if [ "$TESTCALL" = 'testsuccess' ]; then
 		# failure cases can retain partial files and such
-		testempty find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial" -mindepth 1 ! \( -name 'lock' -o -name '*.FAILED' \)
+		testempty cdfind "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial" -mindepth 1 ! \( -name 'lock' -o -name '*.FAILED' \)
 	fi
 	if [ -s "${TMPWORKINGDIRECTORY}/rootdir/var/log/aptgetupdate.before.lst" ]; then
 		testfileequal "${TMPWORKINGDIRECTORY}/rootdir/var/log/aptgetupdate.before.lst" \
-			"$(find "${TMPWORKINGDIRECTORY}/aptarchive/dists" -type f | while read line; do stat --format '%U:%G:%a:%n' "$line"; done | sort)"
+			"$(cdfind "${TMPWORKINGDIRECTORY}/aptarchive/dists" -type f | while read line; do stat --format '%U:%G:%a:%n' "$line"; done | sort)"
 	fi
-	)
 }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; }
 aptautotest_aptcdrom_add() { aptautotest_aptget_update "$@"; }
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