Tidy up

crates/typst-kit/src/package.rs

@@ -193,40 +193,35 @@ impl PackageStorage {
             }
         };
 
-        // Temporary place where the package will be extracted before being
+        // The directory in which the package's version lives.
-        // moved to the target directory. To avoid name clashing we use a PRNG
+        let base_dir = cache_dir.join(format!("{}/{}", spec.namespace, spec.name));
-        // to get a unique directory name.
+
-        let tempdir = Tempdir::create(cache_dir.join(format!(
+        // The place at which the specific package version will live in the end.
-            "{}/{}/.tmp-{}-{}",
+        let package_dir = base_dir.join(format!("{}", spec.version));
-            spec.namespace,
+
-            spec.name,
+        // To prevent multiple Typst instances from interferring, we download
+        // into a temporary directory first and then move this directory to
+        // its final destination.
+        //
+        // In the `rename` function's documentation it is stated:
+        // > This will not work if the new name is on a different mount point.
+        //
+        // By locating the temporary directory directly next to where the
+        // package directory will live, we are (trying our best) making sure
+        // that `tempdir` and `package_dir` are on the same mount point.
+        let tempdir = Tempdir::create(base_dir.join(format!(
+            ".tmp-{}-{}",
             spec.version,
             fastrand::u32(..),
         )))
         .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}"))))?;
 
-        // The place into which we'll move the extracted package.
-        let package_dir =
-            cache_dir.join(format!("{}/{}/{}", spec.namespace, spec.name, spec.version));
-
-        // As to not overcomplicate the code to combat an already rare case
-        // where multiple instances try to download the same package version
-        // concurrently, we are abusing the behavior of the `rename` FS
-        // operation without the help of file locking.
-        //
-        // From the function's documentation it is stated that:
-        //
-        // > This will not work if the new name is on a different mount point.
-        //
-        // By extracting package into the same base directory where all packages
-        // live we are (trying our best) making sure that `extracted_package_dir`
-        // and `package_dir` are on the same mount point.
-        //
         // 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
@@ -234,11 +229,11 @@ impl PackageStorage {
         // 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 the existing moved
+        // This means that we do not check the integrity of an existing moved
-        // package in a very rare case where it is broken, as it does not
+        // package, just like we don't check the integrity if the package
-        // (currently) justify the additional code complexity. If such situation
+        // directory already existed in the first place. If situations with
-        // occur and it will be reported, then we probably would consider a
+        // broken packages still occur even with the rename safeguard, we might
-        // more comprehensive solution (i.e., file locking or checksums).
+        // 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(()),