rework list_files to return something usable

chain/src/txhashset/txhashset.rs

@@ -1522,7 +1522,7 @@ pub fn zip_write(
 	// We expect to see *exactly* the paths listed above.
 	// No attempt is made to be permissive or forgiving with "alternative" paths.
 	// These are the *only* files we will attempt to extract from the zip file.
-	// If any of these are missing the zip extraction will immediately fail.
+	// If any of these are missing we will attempt to continue as some are potentially optional.
 	zip::extract_files(txhashset_data, &txhashset_path, files)?;
 	Ok(())
 }

chain/tests/test_txhashset.rs

@@ -55,94 +55,65 @@ fn test_unexpected_zip() {
 			Path::new(&db_root).join(format!("txhashset_zip_{}", head.hash().to_string())),
 		);
 		// Then add strange files in the original txhashset folder
-		write_file(db_root.clone());
+		File::create(&Path::new(&db_root).join("txhashset").join("badfile"))
+			.expect("problem creating a file");
+		File::create(
+			&Path::new(&db_root)
+				.join("txhashset")
+				.join("output")
+				.join("badfile"),
+		)
+		.expect("problem creating a file");
+
+		let files = file::list_files(&Path::new(&db_root).join("txhashset"));
+		let expected_files: Vec<_> = vec![
+			"badfile",
+			"kernel/pmmr_data.bin",
+			"kernel/pmmr_hash.bin",
+			"kernel/pmmr_size.bin",
+			"output/badfile",
+			"output/pmmr_data.bin",
+			"output/pmmr_hash.bin",
+			"rangeproof/pmmr_data.bin",
+			"rangeproof/pmmr_hash.bin",
+		];
+		assert_eq!(
+			files,
+			expected_files
+				.iter()
+				.map(|x| PathBuf::from(x))
+				.collect::<Vec<_>>()
+		);
+
 		assert!(txhashset::zip_read(db_root.clone(), &head).is_ok());
-		// Check that the temp dir dos not contains the strange files
 		let txhashset_zip_path =
 			Path::new(&db_root).join(format!("txhashset_zip_{}", head.hash().to_string()));
-		assert!(txhashset_contains_expected_files(
-			format!("txhashset_zip_{}", head.hash().to_string()),
-			txhashset_zip_path.clone()
-		));
 		let _ = fs::remove_dir_all(
 			Path::new(&db_root).join(format!("txhashset_zip_{}", head.hash().to_string())),
 		);
-
 		let zip_file = File::open(zip_path).unwrap();
-		assert!(txhashset::zip_write(PathBuf::from(db_root.clone()), zip_file, &head).is_ok());
-		// Check that the txhashset dir dos not contains the strange files
-		let txhashset_path = Path::new(&db_root).join("txhashset");
-		assert!(txhashset_contains_expected_files(
-			"txhashset".to_string(),
-			txhashset_path.clone()
-		));
 		let _ = fs::remove_dir_all(Path::new(&db_root).join("txhashset"));
+		assert!(txhashset::zip_write(PathBuf::from(db_root.clone()), zip_file, &head).is_ok());
+
+		// Check that the new txhashset dir contains *only* the expected files
+		// No "badfiles" and no "size" file.
+		let files = file::list_files(&Path::new(&db_root).join("txhashset"));
+		let expected_files: Vec<_> = vec![
+			"kernel/pmmr_data.bin",
+			"kernel/pmmr_hash.bin",
+			"output/pmmr_data.bin",
+			"output/pmmr_hash.bin",
+			"rangeproof/pmmr_data.bin",
+			"rangeproof/pmmr_hash.bin",
+		];
+		assert_eq!(
+			files,
+			expected_files
+				.iter()
+				.map(|x| PathBuf::from(x))
+				.collect::<Vec<_>>()
+		);
 	}
 	// Cleanup chain directory
 	clean_output_dir(&db_root);
 }
-
-fn write_file(db_root: String) {
-	OpenOptions::new()
-		.create(true)
-		.write(true)
-		.open(
-			Path::new(&db_root)
-				.join("txhashset")
-				.join("kernel")
-				.join("strange0"),
-		)
-		.unwrap();
-	OpenOptions::new()
-		.create(true)
-		.write(true)
-		.open(Path::new(&db_root).join("txhashset").join("strange1"))
-		.unwrap();
-	fs::create_dir(Path::new(&db_root).join("txhashset").join("strange_dir")).unwrap();
-	OpenOptions::new()
-		.create(true)
-		.write(true)
-		.open(
-			Path::new(&db_root)
-				.join("txhashset")
-				.join("strange_dir")
-				.join("strange2"),
-		)
-		.unwrap();
-	fs::create_dir(
-		Path::new(&db_root)
-			.join("txhashset")
-			.join("strange_dir")
-			.join("strange_subdir"),
-	)
-	.unwrap();
-	OpenOptions::new()
-		.create(true)
-		.write(true)
-		.open(
-			Path::new(&db_root)
-				.join("txhashset")
-				.join("strange_dir")
-				.join("strange_subdir")
-				.join("strange3"),
-		)
-		.unwrap();
-}
-
-fn txhashset_contains_expected_files(dirname: String, path_buf: PathBuf) -> bool {
-	let list_zip_files = file::list_files(path_buf.into_os_string().into_string().unwrap());
-	let zip_files_hashset: HashSet<_> = HashSet::from_iter(list_zip_files.iter().cloned());
-	let expected_files = vec![
-		dirname,
-		"output".to_string(),
-		"rangeproof".to_string(),
-		"kernel".to_string(),
-		"pmmr_hash.bin".to_string(),
-		"pmmr_data.bin".to_string(),
-	];
-	let expected_files_hashset = HashSet::from_iter(expected_files.iter().cloned());
-	let intersection: HashSet<_> = zip_files_hashset
-		.difference(&expected_files_hashset)
-		.collect();
-	intersection.is_empty()
-}

util/src/file.rs

@@ -44,18 +44,15 @@ pub fn copy_dir_to(src: &Path, dst: &Path) -> io::Result<u64> {
 }
 
 /// List directory
-pub fn list_files(path: String) -> Vec<String> {
-	let mut files_vec: Vec<String> = vec![];
-	for entry in WalkDir::new(Path::new(&path))
+pub fn list_files(path: &Path) -> Vec<PathBuf> {
+	WalkDir::new(path)
+		.sort_by(|a, b| a.path().cmp(b.path()))
+		.min_depth(1)
 		.into_iter()
-		.filter_map(|e| e.ok())
-	{
-		match entry.file_name().to_str() {
-			Some(path_str) => files_vec.push(path_str.to_string()),
-			None => println!("Could not read optional type"),
-		}
-	}
-	return files_vec;
+		.filter_map(|x| x.ok())
+		.filter(|x| x.file_type().is_file())
+		.filter_map(|x| x.path().strip_prefix(path).map(|x| x.to_path_buf()).ok())
+		.collect()
 }
 
 fn copy_to(src: &Path, src_type: &fs::FileType, dst: &Path) -> io::Result<u64> {

util/src/zip.rs

@@ -34,13 +34,14 @@ pub fn create_zip(dst_file: &File, src_dir: &Path, files: Vec<PathBuf>) -> io::R
 		.unix_permissions(0o644);
 
 	for x in &files {
-		writer.get_mut().start_file_from_path(x, options)?;
 		let file_path = src_dir.join(x);
-		info!("compress: {:?} -> {:?}", file_path, x);
-		let file = File::open(file_path)?;
-		io::copy(&mut BufReader::new(file), &mut writer)?;
-		// Flush the BufWriter after each file so we start then next one correctly.
-		writer.flush()?;
+		if let Ok(file) = File::open(file_path.clone()) {
+			info!("compress: {:?} -> {:?}", file_path, x);
+			writer.get_mut().start_file_from_path(x, options)?;
+			io::copy(&mut BufReader::new(file), &mut writer)?;
+			// Flush the BufWriter after each file so we start then next one correctly.
+			writer.flush()?;
+		}
 	}
 
 	writer.get_mut().finish()?;
@@ -55,24 +56,23 @@ pub fn extract_files(from_archive: File, dest: &Path, files: Vec<PathBuf>) -> io
 	let res = thread::spawn(move || {
 		let mut archive = zip_rs::ZipArchive::new(from_archive).expect("archive file exists");
 		for x in files {
-			let file = archive
-				.by_name(x.to_str().expect("valid path"))
-				.expect("file exists in archive");
-			let path = dest.join(file.sanitized_name());
-			let parent_dir = path.parent().expect("valid parent dir");
-			fs::create_dir_all(&parent_dir).expect("create parent dir");
-			let outfile = fs::File::create(&path).expect("file created");
-			io::copy(&mut BufReader::new(file), &mut BufWriter::new(outfile))
-				.expect("write to file");
+			if let Ok(file) = archive.by_name(x.to_str().expect("valid path")) {
+				let path = dest.join(file.sanitized_name());
+				let parent_dir = path.parent().expect("valid parent dir");
+				fs::create_dir_all(&parent_dir).expect("create parent dir");
+				let outfile = fs::File::create(&path).expect("file created");
+				io::copy(&mut BufReader::new(file), &mut BufWriter::new(outfile))
+					.expect("write to file");
 
-			info!("extract_files: {:?} -> {:?}", x, path);
+				info!("extract_files: {:?} -> {:?}", x, path);
 
-			// Set file permissions to "644" (Unix only).
-			#[cfg(unix)]
-			{
-				use std::os::unix::fs::PermissionsExt;
-				let mode = PermissionsExt::from_mode(0o644);
-				fs::set_permissions(&path, mode).expect("set file permissions");
+				// Set file permissions to "644" (Unix only).
+				#[cfg(unix)]
+				{
+					use std::os::unix::fs::PermissionsExt;
+					let mode = PermissionsExt::from_mode(0o644);
+					fs::set_permissions(&path, mode).expect("set file permissions");
+				}
 			}
 		}
 	})

util/tests/file.rs

@@ -28,11 +28,9 @@ fn copy_dir() {
 	let original_path = Path::new("./target/tmp2/original");
 	let copy_path = Path::new("./target/tmp2/copy");
 	file::copy_dir_to(original_path, copy_path).unwrap();
-	let original_files = file::list_files("./target/tmp2/original".to_string());
-	let copied_files = file::list_files("./target/tmp2/copy".to_string());
-	for i in 1..5 {
-		assert_eq!(copied_files[i], original_files[i]);
-	}
+	let original_files = file::list_files(&Path::new("./target/tmp2/original"));
+	let copied_files = file::list_files(&Path::new("./target/tmp2/copy"));
+	assert_eq!(original_files, copied_files);
 	fs::remove_dir_all(root).unwrap();
 }