Fix apply-patch documentation

docs/src/mbtiles-diff.md

@@ -9,7 +9,7 @@ insertions, and deletions as `NULL` values), for both the tile and metadata tabl
 
 There is one exception: `agg_tiles_hash` metadata value will be renamed to `agg_tiles_hash_after_apply`, and a
 new `agg_tiles_hash` will be generated for the diff file itself. This is done to avoid confusion when applying the diff
-file to the original file, as the `agg_tiles_hash` value will be different after the diff is applied. The `apply-diff`
+file to the original file, as the `agg_tiles_hash` value will be different after the diff is applied. The `apply-patch`
 command will automatically rename the `agg_tiles_hash_after_apply` value back to `agg_tiles_hash` when applying the
 diff.
 
@@ -18,7 +18,7 @@ diff.
 mbtiles diff file1.mbtiles file2.mbtiles diff.mbtiles
 
 # If diff.mbtiles is applied to file1.mbtiles, it will produce file2.mbtiles 
-mbtiles apply-diff file1.mbtiles diff.mbtiles file2a.mbtiles
+mbtiles apply-patch file1.mbtiles diff.mbtiles file2a.mbtiles
 
 # file2.mbtiles and file2a.mbtiles should now be the same
 # Validate both files and see that their hash values are identical

mbtiles/src/bin/mbtiles.rs

@@ -102,11 +102,12 @@ pub struct CopyArgs {
     pub options: SharedCopyOpts,
     /// Compare source file with this file, and only copy non-identical tiles to destination.
     /// Use `mbtiles diff` as a more convenient way to generate this file.
-    /// It should be later possible to run `mbtiles apply-diff` to merge it in.
+    /// Use `mbtiles apply-patch` or `mbtiles copy --apply-patch` to apply the diff file.
     #[arg(long, conflicts_with("apply_patch"))]
     diff_with_file: Option<PathBuf>,
-    /// Compare source file with this file, and only copy non-identical tiles to destination.
-    /// It should be later possible to run `mbtiles apply-diff SRC_FILE DST_FILE` to get the same DIFF file.
+    /// Apply a patch file while copying src to dst.
+    /// Use `mbtiles diff` or `mbtiles copy --diff-with-file` to generate the patch file.
+    /// Use `mbtiles apply-patch` to apply the patch file in-place, without making a copy of the original.
     #[arg(long, conflicts_with("diff_with_file"))]
     apply_patch: Option<PathBuf>,
 }

mbtiles/src/copier.rs

@@ -63,11 +63,8 @@ pub struct MbtilesCopier {
     /// Bounding box to copy, in the format `min_lon,min_lat,max_lon,max_lat`. Can be used multiple times.
     pub bbox: Vec<Bounds>,
     /// Compare source file with this file, and only copy non-identical tiles to destination.
-    /// Use `mbtiles diff` as a more convenient way to generate this file.
-    /// It should be later possible to run `mbtiles apply-diff` to merge it in.
     pub diff_with_file: Option<PathBuf>,
-    /// Compare source file with this file, and only copy non-identical tiles to destination.
-    /// It should be later possible to run `mbtiles apply-diff SRC_FILE DST_FILE` to get the same DIFF file.
+    /// Apply a patch file while copying src to dst.
     pub apply_patch: Option<PathBuf>,
     /// Skip generating a global hash for mbtiles validation. By default, `mbtiles` will compute `agg_tiles_hash` metadata value.
     pub skip_agg_tiles_hash: bool,
@@ -128,7 +125,7 @@ impl MbtileCopierInt {
             (Some(dif_file), None) | (None, Some(dif_file)) => {
                 let dif_mbt = Mbtiles::new(dif_file)?;
                 let dif_type = dif_mbt.open_and_detect_type().await?;
-                Some((dif_mbt, dif_type, dif_type))
+                Some((dif_mbt, dif_type))
             }
             (Some(_), Some(_)) => unreachable!(), // validated in the Self::new
             _ => None,
@@ -157,7 +154,7 @@ impl MbtileCopierInt {
             CopyType::Metadata => "metadata ",
         };
         let dst_type: MbtType;
-        if let Some((dif_mbt, dif_type, _)) = &dif {
+        if let Some((dif_mbt, dif_type)) = &dif {
             if !is_empty_db {
                 return Err(MbtError::NonEmptyTargetFile(self.options.dst_file));
             }
@@ -183,9 +180,9 @@ impl MbtileCopierInt {
             self.init_new_schema(&mut conn, src_type, dst_type).await?;
         }
 
+        // SAFETY: This must be scoped to make sure the handle is dropped before we continue using conn
+        // Make sure not to execute any other queries while the handle is locked
         {
-            // SAFETY: This must be scoped to make sure the handle is dropped before we continue using conn
-            // Make sure not to execute any other queries while the handle is locked
             let mut handle_lock = conn.lock_handle().await?;
             let handle = handle_lock.as_raw_handle().as_ptr();
 
@@ -205,7 +202,7 @@ impl MbtileCopierInt {
             }
 
             if self.options.copy.copy_metadata() {
-                self.copy_metadata(&rusqlite_conn, dif.as_ref(), on_duplicate)?;
+                self.copy_metadata(&rusqlite_conn, dif.is_some(), on_duplicate)?;
             } else {
                 debug!("Skipping copying metadata");
             }
@@ -215,22 +212,24 @@ impl MbtileCopierInt {
             dst_mbt.update_agg_tiles_hash(&mut conn).await?;
         }
 
+        if dif.is_some() {
+            detach_db(&mut conn, "diffDb").await?;
+        }
+
         detach_db(&mut conn, "sourceDb").await?;
-        // Ignore error because we might not have attached diffDb
-        let _ = detach_db(&mut conn, "diffDb").await;
 
         Ok(conn)
     }
 
     fn copy_metadata(
         &self,
         rusqlite_conn: &Connection,
-        dif: Option<&(Mbtiles, MbtType, MbtType)>,
+        is_diff: bool,
         on_duplicate: CopyDuplicateMode,
     ) -> Result<(), MbtError> {
         let on_dupl = on_duplicate.to_sql();
         let sql;
-        if dif.is_some() {
+        if is_diff {
             // Insert all rows from diffDb.metadata if they do not exist or are different in sourceDb.metadata.
             // Also insert all names from sourceDb.metadata that do not exist in diffDb.metadata, with their value set to NULL.
             // Rename agg_tiles_hash to agg_tiles_hash_after_apply because agg_tiles_hash will be auto-added later
@@ -282,14 +281,14 @@ impl MbtileCopierInt {
     fn copy_tiles(
         &self,
         rusqlite_conn: &Connection,
-        dif: Option<&(Mbtiles, MbtType, MbtType)>,
+        dif: Option<&(Mbtiles, MbtType)>,
         src_type: MbtType,
         dst_type: MbtType,
         on_duplicate: CopyDuplicateMode,
     ) -> Result<(), MbtError> {
         let on_dupl = on_duplicate.to_sql();
 
-        let select_from = if let Some((_, dif_type, _)) = &dif {
+        let select_from = if let Some((_, dif_type)) = &dif {
             if self.options.diff_with_file.is_some() {
                 Self::get_select_from_with_diff(*dif_type, dst_type)
             } else {

mbtiles/src/patcher.rs

@@ -14,60 +14,53 @@ pub async fn apply_patch(base_file: PathBuf, patch_file: PathBuf) -> MbtResult<(
 
     let mut conn = base_mbt.open().await?;
     let base_type = base_mbt.detect_type(&mut conn).await?;
-    patch_mbt.attach_to(&mut conn, "patchDb").await?;
 
     info!("Applying patch file {patch_mbt} ({patch_type}) to {base_mbt} ({base_type})");
+
+    patch_mbt.attach_to(&mut conn, "patchDb").await?;
     let select_from = get_select_from(base_type, patch_type);
     let (main_table, insert1, insert2) = get_insert_sql(base_type, select_from);
 
-    query(&format!("{insert1} WHERE tile_data NOTNULL"))
-        .execute(&mut conn)
-        .await?;
+    let sql = format!("{insert1} WHERE tile_data NOTNULL");
+    query(&sql).execute(&mut conn).await?;
 
     if let Some(insert2) = insert2 {
-        query(&format!("{insert2} WHERE tile_data NOTNULL"))
-            .execute(&mut conn)
-            .await?;
+        let sql = format!("{insert2} WHERE tile_data NOTNULL");
+        query(&sql).execute(&mut conn).await?;
     }
 
-    query(&format!(
+    let sql = format!(
         "
     DELETE FROM {main_table}
     WHERE (zoom_level, tile_column, tile_row) IN (
         SELECT zoom_level, tile_column, tile_row FROM ({select_from} WHERE tile_data ISNULL)
     )"
-    ))
-    .execute(&mut conn)
-    .await?;
+    );
+    query(&sql).execute(&mut conn).await?;
 
     if base_type.is_normalized() {
         debug!("Removing unused tiles from the images table (normalized schema)");
-        query("DELETE FROM images WHERE tile_id NOT IN (SELECT tile_id FROM map)")
-            .execute(&mut conn)
-            .await?;
+        let sql = "DELETE FROM images WHERE tile_id NOT IN (SELECT tile_id FROM map)";
+        query(sql).execute(&mut conn).await?;
     }
 
     // Copy metadata from patchDb to the destination file, replacing existing values
     // Convert 'agg_tiles_hash_in_patch' into 'agg_tiles_hash'
     // Delete metadata entries if the value is NULL in patchDb
-    query(&format!(
+    let sql = format!(
         "
     INSERT OR REPLACE INTO metadata (name, value)
     SELECT IIF(name = '{AGG_TILES_HASH_AFTER_APPLY}', '{AGG_TILES_HASH}', name) as name,
            value
     FROM patchDb.metadata
     WHERE name NOTNULL AND name != '{AGG_TILES_HASH}';"
-    ))
-    .execute(&mut conn)
-    .await?;
+    );
+    query(&sql).execute(&mut conn).await?;
 
-    query(
-        "
+    let sql = "
     DELETE FROM metadata
-    WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);",
-    )
-    .execute(&mut conn)
-    .await?;
+    WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);";
+    query(sql).execute(&mut conn).await?;
 
     detach_db(&mut conn, "patchDb").await
 }

mbtiles/src/validation.rs

@@ -24,8 +24,7 @@ use crate::{invert_y_value, Mbtiles};
 /// Metadata key for the aggregate tiles hash value
 pub const AGG_TILES_HASH: &str = "agg_tiles_hash";
 
-/// Metadata key for a diff file,
-/// describing the eventual [`AGG_TILES_HASH`] value once the diff is applied
+/// Metadata key for a diff file, describing the eventual [`AGG_TILES_HASH`] value of the resulting tileset once the diff is applied
 pub const AGG_TILES_HASH_AFTER_APPLY: &str = "agg_tiles_hash_after_apply";
 
 #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, EnumDisplay, Serialize)]
@@ -116,7 +115,7 @@ impl Mbtiles {
             tested_zoom = r.zoom_level.unwrap_or(-1);
         }
 
-        // Afterwards, iterate over tiles in all allowed zooms and check for consistency
+        // Afterward, iterate over tiles in all allowed zooms and check for consistency
         for z in tilejson.minzoom.unwrap_or(0)..=tilejson.maxzoom.unwrap_or(18) {
             if i64::from(z) == tested_zoom {
                 continue;

mbtiles/tests/copy.rs

@@ -5,7 +5,6 @@ use std::str::from_utf8;
 use ctor::ctor;
 use insta::{allow_duplicates, assert_snapshot};
 use itertools::Itertools as _;
-use log::info;
 use martin_tile_utils::xyz_to_bbox;
 use mbtiles::AggHashType::Verify;
 use mbtiles::IntegrityCheckType::Off;
@@ -105,14 +104,14 @@ async fn open(file: &str) -> MbtResult<(Mbtiles, SqliteConnection)> {
 
 /// Run [`MbtilesCopier`], the first two params are source and destination [`Mbtiles`] refs, the rest are optional (key => val)* params.
 macro_rules! copy {
-    ($src_path:expr, $dst_path:expr $( , $key:tt => $val:expr )* $(,)?) => {{
+    ($src_path:expr, $dst_path:expr $( , $key:tt => $val:expr )* $(,)?) => {
         MbtilesCopier {
             src_file: $src_path,
             dst_file: $dst_path
             $(, $key : $val)*,
             ..Default::default()
         }.run().await.unwrap()
-    }};
+    };
 }
 
 /// Same as the copy! macro, but with the result dumped.
@@ -509,7 +508,6 @@ async fn patch_on_copy(
     let v2 = v2_type.map_or("dflt", shorten);
     let prefix = format!("{v1}+{dif}={v2}");
 
-    info!("TEST: Compare v1 with v2, and copy anything that's different (i.e. mathematically: v2-v1=diff)");
     let (v2_mbt, mut v2_cn) = open!(patch_on_copy, "{prefix}__v2");
     copy! {
         databases.path("v1", v1_type),