All folders get their original modification time back on a restore

The backup records the time of the folders with something in them, packed in the meta record,
so an older version still reads the snapshot. A restore through Nextcloud leaves a folder that
was already there alone. The round trip checks what Nextcloud shows, not only the disk.

CHANGELOG.md

@@ -22,6 +22,9 @@ shown under 1.0.0. The git tag of a release candidate goes after the date.
 - A schedule per location: off, once a day, or every N hours from a given time, on the page under
   Backups or with `occ backup:target:schedule`. After the update every location keeps the time that
   held for all of them; a new location starts with the schedule of the first one.
+- All folders get their original modification time back on a restore, not only the empty ones, from
+  the first backup made with this version. An older version of NG Backup can still read new restore
+  points. A restore through Nextcloud into a folder that is already there leaves its time alone.
 
 ### Changed
 - Locations are backed up one after the other: a backup that has to wait shows as queued. A restore

README.md

@@ -293,8 +293,9 @@ On the backlog; know them before you rely on NG Backup:
 
 - **Empty folders only come back from backups made with 1.0.3 or newer.** Older restore points do
   not contain them.
-- **Folders with files in them show the time of the restore**, not their original modification time.
+- **Folders with files in them only get their original modification time back from backups made with
-  The files keep theirs. On the roadmap.
+  1.1.0 or newer.** From older restore points they show the time of the restore; the files keep theirs.
+  A restore through Nextcloud into a folder that is already there leaves that folder's time alone.
 - **Only the data directory is backed up.** Object storage as primary storage (`objectstore` in
   `config.php`) is not supported: a backup refuses to start there. Folders mounted through the
   External storage app are not in the data directory and are **not included**, without a warning.

lib/Job/BackupRun.php

@@ -52,14 +52,30 @@ final class BackupRun {
 			$stats = $state['stats'];
 			$walker = new TreeWalker($state['roots'], $state['exclude'], function (string $logical) use (&$state, &$stats): void {
 				self::recordUnreadable($state, $stats, $logical);
-			});
+			}, true);
 			$parent = new ParentCursor($repo, $state['parent'], $state['parentPos']);
 			$lines = [];
 			$emptyDirs = [];
+			$folders = [];
 			$finishedWalk = true;
 
 			foreach ($walker->walk($state['after']) as $logical => $path) {
-				if (!mb_check_encoding($logical, 'UTF-8') || !Repository::isSafePath($logical)) {
+				$safe = mb_check_encoding($logical, 'UTF-8') && Repository::isSafePath($logical);
+				if ($walker->atFolderWithContent()) {
+					// Only its time: what is in it is reported by itself, and a bad name in the path of
+					// each of those is what gets reported. The cursor stays for such a name, see below.
+					$mtime = $safe ? @filemtime($path) : false;
+					if ($safe && $mtime !== false) {
+						$folders[] = json_encode([$logical, $mtime], JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR);
+						$state['after'] = $logical;
+						if (microtime(true) >= $deadline) {
+							$finishedWalk = false;
+							break;
+						}
+					}
+					continue;
+				}
+				if (!$safe) {
 					// A name a snapshot cannot hold (a backslash, a control character, not UTF-8). One
 					// such path must not make a backup fail, or the whole snapshot unreadable later.
 					// The cursor stays where it is: the state is JSON, which cannot hold such a name. A
@@ -164,6 +180,11 @@ final class BackupRun {
 				$writer->write(implode("\n", $emptyDirs) . "\n");
 				$state['dirSegments'][] = $writer->finish();
 			}
+			if ($folders !== []) {
+				$writer = $repo->blobWriter($stats);
+				$writer->write(implode("\n", $folders) . "\n");
+				$state['folderSegments'][] = $writer->finish();
+			}
 			$repo->flushPacks();
 			$state['parentPos'] = $parent->position();
 			$state['stats'] = $stats;
@@ -186,10 +207,14 @@ final class BackupRun {
 					$emptyDirs[] = json_decode($line, true, 8, JSON_THROW_ON_ERROR);
 				}
 			}
+			// The times of the other folders too, packed: every look at a snapshot's meta record reads
+			// this, and a string of a few hundred KiB costs that far less than tens of thousands of arrays.
+			$folders = isset($state['folderSegments']) ? implode("\n", iterator_to_array($repo->readLines(array_values(array_merge(...$state['folderSegments']))), false)) : '';
 			$repo->putSnapshot($state['snapshot'],
 				['time' => $state['started'], 'parent' => $state['parent'], 'label' => $state['label'],
 					'roots' => array_keys($state['roots']), 'stats' => $state['stats'],
-					'warnings' => self::warnings($state)] + ($emptyDirs === [] ? [] : ['emptyDirs' => $emptyDirs]) + $state['meta'],
+					'warnings' => self::warnings($state)] + ($emptyDirs === [] ? [] : ['emptyDirs' => $emptyDirs])
+					+ ($folders === '' ? [] : ['folderTimes' => base64_encode((string)gzdeflate($folders))]) + $state['meta'],
 				$repo->readLines(array_values(array_merge(...$state['segments']))));
 			$state['phase'] = 'commit';
 		}

lib/Job/TreeWalker.php

@@ -17,6 +17,8 @@ namespace OCA\NgBackup\Job;
  * and be merge-joined with the previous snapshot without holding any list in memory.
  */
 final class TreeWalker {
+	private bool $atFolder = false;
+
 	/**
 	 * @param array<string, string> $roots name => absolute directory, walked in this order
 	 * @param list<string> $exclude path prefixes ("<root>/<relative>") that are skipped entirely;
@@ -27,9 +29,16 @@ final class TreeWalker {
 		private array $exclude = [],
 		/** Called with the logical path of a directory that cannot be listed. */
 		private ?\Closure $onUnreadable = null,
+		/** Also yield directories with something in them, right before the first thing in them. */
+		private bool $withFolders = false,
 	) {
 	}
 
+	/** Whether what was just yielded is a directory with something in it (see $withFolders). */
+	public function atFolderWithContent(): bool {
+		return $this->atFolder;
+	}
+
 	/** @return \Generator<string, string> logical path => absolute path, strictly after $after */
 	public function walk(?string $after = null): \Generator {
 		foreach ($this->roots as $name => $dir) {
@@ -75,7 +84,14 @@ final class TreeWalker {
 			if (is_link($childAbs)) {
 				continue; // never follow symlinks out of the data directory
 			}
-			if (is_dir($childAbs)) {
+			$childIsDir = is_dir($childAbs);
+			if (($childIsDir || is_file($childAbs)) && $empty && !$this->isExcluded($childLogical, $childIsDir)
+				&& $this->withFolders && $logical !== '' && ($after === null || self::compare($logical, $after) > 0)) {
+				$this->atFolder = true;
+				yield $logical => $abs;
+				$this->atFolder = false;
+			}
+			if ($childIsDir) {
 				$empty = $empty && $this->isExcluded($childLogical, true);
 				// Skip a directory entirely if everything in it sorts before the cursor.
 				if ($after !== null && self::compare($childLogical, $after) < 0 && !self::isPrefix($childLogical, $after)) {

lib/Repository/Repository.php

@@ -435,9 +435,41 @@ final class Repository {
 				self::restoreEmptyDir($target, $dest, $path, $mtime);
 			}
 		}
+		// Very last, nothing is written in them any more. Also on a run that goes on after a stop, and
+		// the deepest first. The time is not worth stopping for: all the files are there by now.
+		foreach (array_reverse($this->folderTimes($snapshotId)) as [$path, $mtime]) {
+			if (!self::underPrefix($path, $prefix) || ($stripPrefix && $path === rtrim($prefix, '/'))) {
+				continue; // the target itself is not ours to change
+			}
+			if ($heartbeat !== null) {
+				$heartbeat();
+			}
+			$dest = self::destination($target, $path, $prefix, $stripPrefix);
+			@touch(self::directoryInTarget($target, $dest, $path), $mtime);
+		}
 		return $count;
 	}
 
+	/**
+	 * The folders with something in them that a snapshot recorded, parents before children (none
+	 * before 1.1.0). Damage here is no reason to give up on the files of a restore.
+	 *
+	 * @return list<array{0: string, 1: int}> path, modification time
+	 */
+	public function folderTimes(string $snapshotId): array {
+		$meta = $this->lastMeta[$snapshotId] ?? $this->snapshotMeta($snapshotId);
+		$packed = is_string($meta['folderTimes'] ?? null) ? base64_decode($meta['folderTimes'], true) : false;
+		$plain = $packed === false ? false : @gzinflate($packed);
+		$folders = [];
+		foreach ($plain === false ? [] : explode("\n", $plain) as $line) {
+			$folder = json_decode($line, true);
+			if (is_array($folder) && is_string($folder[0] ?? null) && self::isSafePath($folder[0]) && is_int($folder[1] ?? null)) {
+				$folders[] = [$folder[0], $folder[1]];
+			}
+		}
+		return $folders;
+	}
+
 	/**
 	 * The folders without anything in them that a snapshot recorded (none before 1.0.3).
 	 *

lib/Service/RestoreService.php

@@ -155,6 +155,17 @@ final class RestoreService {
 		$dest = $this->ensureFolder($userFolder, $parentRel);
 		$names = $single ? [basename($rel) => $entries['']] : $entries;
 
+		// The folders with something in them that this restore creates get their time back; one that
+		// was already there keeps its own, as its content changes now. Noted before anything is written.
+		/** @var array<array-key, int> $newFolders a folder called "2026" is an integer key */
+		$newFolders = [];
+		foreach ($single ? [] : $repo->folderTimes($snapshotId) as [$dir, $mtime]) {
+			$relative = $dir === $source ? '' : (str_starts_with($dir, $source . '/') ? substr($dir, strlen($source) + 1) : null);
+			if ($relative !== null && ($relative === '' ? !$destExisted : !$dest->nodeExists($relative))) {
+				$newFolders[$relative] = $mtime;
+			}
+		}
+
 		$this->cleaned = [];
 		$restored = 0;
 		foreach ($names as $relPath => $entry) {
@@ -190,6 +201,13 @@ final class RestoreService {
 		if ($mode === self::MODE_REPLACE && !$single) {
 			$trashed = $this->trashExtras($dest, '', array_flip(array_keys($names)) + $emptyDirs);
 		}
+		// Last and deepest first: a change in a folder also touches the folders above it.
+		foreach (array_reverse($newFolders, true) as $dir => $mtime) {
+			$this->ensureFolder($dest, (string)$dir)->touch($mtime);
+			if ($progress !== null) {
+				$progress($restored, count($names), (string)$dir); // keeps the lease alive too
+			}
+		}
 		return ['restored' => $restored, 'trashed' => $trashed, 'target' => $uid . ':' . $userFolder->getRelativePath($dest->getPath())];
 	}
 

tests/Unit/Repository/FolderTimesTest.php

@@ -0,0 +1,127 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\NgBackup\Tests\Unit\Repository;
+
+use OCA\NgBackup\Backend\LocalBackend;
+use OCA\NgBackup\Crypto\KeyRing;
+use OCA\NgBackup\Job\BackupRun;
+use OCA\NgBackup\Repository\Repository;
+use OCA\NgBackup\Tests\Unit\TempDirTrait;
+use PHPUnit\Framework\TestCase;
+
+/** Folders with something in them get their original modification time back on a restore. */
+class FolderTimesTest extends TestCase {
+	use TempDirTrait;
+
+	private const OLD = 1500000000;
+	private const OLDER = 1400000000;
+
+	private LocalBackend $backend;
+	private KeyRing $keys;
+	private string $src;
+
+	protected function setUp(): void {
+		$this->backend = new LocalBackend($this->tempDir());
+		$this->keys = KeyRing::generate();
+		Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw'));
+		$this->src = $this->tempDir();
+		mkdir("$this->src/data/alice/files/Old/Deep", 0700, true);
+		mkdir("$this->src/data/alice/files/2026", 0700, true);
+		file_put_contents("$this->src/data/alice/files/Old/Deep/a.txt", 'a');
+		file_put_contents("$this->src/data/alice/files/Old/b.txt", 'b');
+		file_put_contents("$this->src/data/alice/files/2026/c.txt", 'c');
+		touch("$this->src/data/alice/files/Old/Deep", self::OLDER);
+		touch("$this->src/data/alice/files/Old", self::OLD);
+		touch("$this->src/data/alice/files/2026", self::OLD);
+	}
+
+	private function repo(): Repository {
+		return Repository::openWithKey($this->backend, $this->keys);
+	}
+
+	private function backup(float $seconds = 30.0): string {
+		$state = BackupRun::start(['data' => "$this->src/data"]);
+		for ($i = 0; $i < 500 && $state['phase'] !== 'commit'; $i++) {
+			$state = json_decode(json_encode(BackupRun::step($this->repo(), $state, microtime(true) + $seconds)), true);
+		}
+		$this->assertSame('commit', $state['phase']);
+		$this->repo()->recordSnapshot($state['snapshot']);
+		return $state['snapshot'];
+	}
+
+	private function time(string $path): int {
+		clearstatcache();
+		return (int)filemtime($path);
+	}
+
+	public function testEachFolderIsRecordedOnceAlsoInSmallSteps(): void {
+		$expected = ['data', 'data/alice', 'data/alice/files', 'data/alice/files/2026', 'data/alice/files/Old', 'data/alice/files/Old/Deep'];
+		$this->assertSame($expected, array_column($this->repo()->folderTimes($this->backup()), 0));
+		$this->assertSame($expected, array_column($this->repo()->folderTimes($this->backup(0.0)), 0), 'a step per entry');
+	}
+
+	public function testNoEmptyFolderIsAmongThem(): void {
+		mkdir("$this->src/data/alice/files/Hollow");
+		$snapshot = $this->backup();
+		$this->assertNotContains('data/alice/files/Hollow', array_column($this->repo()->folderTimes($snapshot), 0));
+		$this->assertContains('data/alice/files/Hollow', array_column($this->repo()->emptyDirs($snapshot), 0));
+	}
+
+	public function testARestoreBringsThemBack(): void {
+		$snapshot = $this->backup();
+		$target = $this->tempDir();
+		$this->repo()->restore($snapshot, $target);
+		$this->assertSame(self::OLD, $this->time("$target/data/alice/files/Old"));
+		$this->assertSame(self::OLDER, $this->time("$target/data/alice/files/Old/Deep"));
+		$this->assertSame(self::OLD, $this->time("$target/data/alice/files/2026"));
+	}
+
+	public function testARestoreThatGoesOnAfterAStopToo(): void {
+		$snapshot = $this->backup();
+		$target = $this->tempDir();
+		$this->repo()->restore($snapshot, $target);
+		touch("$target/data/alice/files/Old", 1800000000);
+		$this->repo()->restore($snapshot, $target, keepUnchanged: true);
+		$this->assertSame(self::OLD, $this->time("$target/data/alice/files/Old"));
+	}
+
+	public function testAPrefixLeavesTheTargetAndOthersAlone(): void {
+		$snapshot = $this->backup();
+		$target = $this->tempDir();
+		$this->repo()->restore($snapshot, $target, 'data/alice/files/Old/', true);
+		$this->assertSame(self::OLDER, $this->time("$target/Deep"));
+		$this->assertNotSame(self::OLD, $this->time($target), 'the directory that was chosen is not ours to change');
+		$this->assertDirectoryDoesNotExist("$target/2026");
+	}
+
+	public function testALinkOutOfTheTargetIsRefused(): void {
+		$snapshot = $this->backup();
+		$target = $this->tempDir();
+		$outside = $this->tempDir();
+		touch($outside, 1800000000);
+		mkdir("$target/data/alice/files", 0700, true);
+		symlink($outside, "$target/data/alice/files/Old");
+		$this->expectException(\OCA\NgBackup\Repository\RestoreTargetException::class);
+		$this->repo()->restore($snapshot, $target);
+	}
+
+	public function testAFolderWithABadNameIsLeftOutAndWhatIsInItReported(): void {
+		mkdir("$this->src/data/odd\\folder");
+		file_put_contents("$this->src/data/odd\\folder/x", 'x');
+		$state = BackupRun::start(['data' => "$this->src/data"]);
+		for ($i = 0; $i < 50 && $state['phase'] !== 'commit'; $i++) {
+			$state = json_decode(json_encode(BackupRun::step($this->repo(), $state, 0.0), JSON_THROW_ON_ERROR), true, 512, JSON_THROW_ON_ERROR);
+		}
+		$this->assertSame('commit', $state['phase']);
+		$this->assertSame(1, $state['stats']['badNames']);
+		$this->repo()->recordSnapshot($state['snapshot']);
+		$this->assertNotContains("data/odd\\folder", array_column($this->repo()->folderTimes($state['snapshot']), 0));
+	}
+
+	public function testASnapshotWithoutThemHasNone(): void {
+		$snapshot = $this->repo()->backupDirectory($this->src)['snapshot'];
+		$this->assertSame([], $this->repo()->folderTimes($snapshot));
+	}
+}

tests/integration/roundtrip.sh

@@ -23,12 +23,15 @@ ok "key, kit code and confirmation"
 occ backup:target:add ci $TARGET_ARGS --path=repo >/dev/null || fail "target add"
 ok "location added"
 
-# Test data: a user with files.
+# Test data: a user with files. No skeleton files on the first login, below, in the middle of the checks.
+occ config:system:set skeletondirectory --value='' >/dev/null
+occ config:system:set templatedirectory --value='' >/dev/null
 $X sh -c "OC_PASS='Roundtrip-pw-2026' php occ user:add --password-from-env $U >/dev/null"
 # large.bin (40 MB) fills more than one pack: a restore to a directory reads ahead over several ranges
 $X sh -c "mkdir -p $DATA/$U/files/Docs/Sub $DATA/$U/files/Docs/Empty/Nested $DATA/$U/files/Hollow && touch -d @1700000000 $DATA/$U/files/Hollow && for i in 1 2 3 4 5; do head -c 300000 /dev/urandom > $DATA/$U/files/Docs/f\$i.bin; done; echo hello > $DATA/$U/files/Docs/Sub/note.txt; head -c 6000000 /dev/urandom > $DATA/$U/files/big.bin; head -c 40000000 /dev/urandom > $DATA/$U/files/large.bin"
+$X sh -c "touch -d @1600000000 $DATA/$U/files/Docs/Sub $DATA/$U/files/Docs"
 occ files:scan "$U" >/dev/null
-SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum")
+SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum")
 
 occ backup:run ci -l first >/dev/null || fail "first backup"
 SECOND=$(occ backup:run ci -l second)
@@ -44,6 +47,14 @@ occ files:scan "$U" >/dev/null
 occ backup:restore ci "$SNAP" "data/$U/files/Docs" -m new-folder >/dev/null || fail "restore new-folder"
 $X sh -c "ls $DATA/$U/files | grep -q '^Restored '" || fail "no Restored folder"
 $X sh -c "test -d $DATA/$U/files/Restored\ */Docs/Empty/Nested" || fail "the empty folder is not in the Restored folder"
+# Folders with files: on disk and in what Nextcloud shows, the parent as well as the child.
+RDIR=$($X sh -c "cd $DATA/$U/files && ls -d Restored\ *")
+[ "$($X sh -c "stat -c %Y $DATA/$U/files/Restored\ */Docs/Sub")" = 1600000000 ] || fail "a folder with files did not get its time back"
+[ "$($X sh -c "stat -c %Y $DATA/$U/files/Restored\ */Docs")" = 1600000000 ] || fail "the parent folder did not get its time back"
+DAV=$($X curl -sf -u "$U:Roundtrip-pw-2026" -X PROPFIND -H 'Depth: 1' "http://localhost/remote.php/dav/files/$U/$(php -r 'echo rawurlencode($argv[1]);' "$RDIR")/Docs/") || fail "PROPFIND"
+[ "$(echo "$DAV" | grep -o 'Sun, 13 Sep 2020 12:26:40 GMT' | wc -l)" = 2 ] || fail "Nextcloud does not show the time of the folders: $(echo "$DAV" | grep -o '<d:getlastmodified>[^<]*' | sort | uniq -c)"
+occ backup:restore ci "$SNAP" "data/$U/files/Docs/Sub" -m merge >/dev/null || fail "restore into a folder that is there"
+[ "$($X stat -c %Y "$DATA/$U/files/Docs/Sub")" != 1600000000 ] || fail "a folder that was already there got the old time"
 occ backup:browse ci "$SNAP" "data/$U/files" | grep Hollow >/dev/null || fail "an empty folder is not shown when browsing the snapshot"
 occ backup:restore ci "$SNAP" "data/$U/files/Hollow" -m merge >/dev/null || fail "restore of a folder that was empty"
 $X sh -c "test -d $DATA/$U/files/Hollow" || fail "the empty folder did not come back by itself"
@@ -52,8 +63,8 @@ occ backup:restore ci "$SNAP" "data/$U/files/Docs" -m replace >/dev/null || fail
 $X sh -c "test -d $DATA/$U/files/Docs/Empty/Nested" || fail "replace did not bring back the empty folder, or moved it to the trash"
 ok "empty folders: restored in each mode, by themselves and shown when browsing"
 $X sh -c "rm -rf '$DATA/$U/files/Restored '*" ; occ files:scan "$U" >/dev/null
-SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum")
+SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum")
-[ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after restore"
+[ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after restore: $(diff <(echo "$SUM_BEFORE") <(echo "$SUM_AFTER") | cut -c1-120)"
 $X sh -c "ls $DATA/$U/files_trashbin/files/ | grep -q '^extra.txt'" || fail "extra file not in trash"
 ok "restore new-folder and replace: files identical, extra file in the trash"
 
@@ -67,7 +78,7 @@ ok "restore of one file to a directory; the temporary of a stopped restore remov
 
 # All of the user's files to a directory: over S3 and SFTP the packs are read ahead
 occ backup:restore ci "$SNAP" "data/$U/files" -d /tmp/ngb-rt-dir >/dev/null || fail "restore of a folder to a directory"
-SUM_DIR=$($X sh -c "cd /tmp/ngb-rt-dir/data/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum")
+SUM_DIR=$($X sh -c "cd /tmp/ngb-rt-dir/data/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum")
 [ "$SUM_BEFORE" = "$SUM_DIR" ] || fail "the files restored to a directory differ"
 $X sh -c "ls -A /tmp/ngb-rt-dir | grep -q ngb-restore-spool" && fail "the read-ahead spool was left in the directory"
 $X sh -c "test -d /tmp/ngb-rt-dir/data/$U/files/Docs/Empty/Nested" || fail "the empty folder is not in the directory"