Repository: typst/typst
File: crates/typst-kit/src/package.rs (Rust)

Code before:
        };

        create_dir(package_download_dir.as_path())?;
        create_dir(package_dir)?;

        let decompressed = flate2::read::GzDecoder::new(data.as_slice());
        tar::Archive::new(decompressed)
            .unpack(&package_download_dir)
            .map_err(|err| {
                fs::remove_dir_all(package_dir).ok();
                PackageError::MalformedArchive(Some(eco_format!("{err}")))
            })?;

        let removed_download_dir = || {
            fs::remove_dir_all(&package_download_dir).map_err(|err| {
                PackageError::Other(Some(eco_format!(
                    "failed to delete temporary package directory: {err}"
                )))
            })
        };

        // - Assuming both paths are from the same mount point.
        // - DirectoryNotEmpty error is assumed to happen only when another instance
        //   already moved the package to the destination directory, therefore it is
        //   safe to ignore it.
        match fs::rename(&package_download_dir, package_dir) {
            Ok(()) => Ok(()),
            Err(err) if err.kind() == std::io::ErrorKind::DirectoryNotEmpty => {
                removed_download_dir()
            }
            Err(err) => {
                removed_download_dir()?;
                Err(PackageError::Other(Some(eco_format!(
                    "failed to move the downloaded package directory: {err}"
                ))))
            }
        }
    }
}

/// Minimal information required about a package to determine its latest
/// version.
#[derive(Deserialize)]
struct MinimalPackageInfo {
    name: String,
    version: PackageVersion,
}

#[cfg(test)]
mod tests {
    use super::*;

Review comment (suggestion):
I think we should have a more elaborate comment which explains the why, we assume the same mount point, i.e. the atomicity of `rename` on *nix file systems.

As far as I recall it, we landed on `rename` is atomic on *nix when `src` and `dst` are on the same file system and the file system is not arcane. I don't recall how it behaves on Windows itself.

Code after:
        .map_err(|err| error("failed to create temporary package directory", err))?;

        // Decompress the archive into the temporary directory.
        let decompressed = flate2::read::GzDecoder::new(data.as_slice());
        tar::Archive::new(decompressed)
            .unpack(&tempdir)
            .map_err(|err| PackageError::MalformedArchive(Some(eco_format!("{err}"))))?;

        // When trying to move (i.e., `rename`) the directory from one place to
        // another and the target/destination directory is empty, then the
        // operation will succeed (if it's atomic, or hardware doesn't fail, or
        // power doesn't go off, etc.). If however the target directory is not
        // empty, i.e., another instance already successfully moved the package,
        // then we can safely ignore the `DirectoryNotEmpty` error.
        //
        // This means that we do not check the integrity of an existing moved
        // package, just like we don't check the integrity if the package
        // directory already existed in the first place. If situations with
        // broken packages still occur even with the rename safeguard, we might
        // consider more complex solutions like file locking or checksums.
        match fs::rename(&tempdir, &package_dir) {
            Ok(()) => Ok(()),
            Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(()),
            Err(err) => Err(error("failed to move downloaded package directory", err)),
        }
    }
}

/// Minimal information required about a package to determine its latest
/// version.
#[derive(Deserialize)]
struct MinimalPackageInfo {
    name: String,
    version: PackageVersion,
}

/// A temporary directory that is a automatically cleaned up.
struct Tempdir(PathBuf);

impl Tempdir {
    /// Creates a directory at the path and auto-cleans it.
    fn create(path: PathBuf) -> io::Result<Self> {
        std::fs::create_dir_all(&path)?;
        Ok(Self(path))
    }
}

impl Drop for Tempdir {
    fn drop(&mut self) {
        _ = fs::remove_dir_all(&self.0);
    }