diff --git a/src/db.rs b/src/db.rs index 60af07cbb..9fce2d177 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1637,11 +1637,34 @@ impl Database { ) .await?; if let Ok(Some(row)) = rows.next().await { - // Found it! Use this ID. We'll update its fields below. let id: i64 = row.get(0)?; + let existing_subject: String = row.get(3)?; let subject_index: u32 = row.get(4).unwrap_or(9999); let existing_total: u32 = row.get(5).unwrap_or(1); + let v_new = version.unwrap_or(1); + let v_old = crate::patch::parse_subject_version(&existing_subject).unwrap_or(1); + + // Check for index collision + let index_collision = if part_index == 0 { + subject_index == 0 + } else { + let mut p_rows = self + .conn + .query( + "SELECT 1 FROM patches WHERE patchset_id = ? AND part_index = ? AND message_id != ?", + libsql::params![id, part_index, message_id], + ) + .await?; + p_rows.next().await.ok().flatten().is_some() + }; + + if index_collision || v_new != v_old { + continue; + } + + // Found it! Use this ID. We'll update its fields below. + // Prevent downgrading a series to a singleton if we already have multiple parts. // This handles cases where a singleton root (1/1) overwrites a series (N/N) inferred from replies. let final_total = if total_parts == 1 && existing_total > 1 { @@ -1839,8 +1862,10 @@ impl Database { }; // Thread Enforcement: To prevent cross-thread "stealing" of patches for resends of the same series, - // we strictly require multi-part series patches to belong to the same thread. - let thread_compatible = same_thread || is_singleton; + // we strictly require multi-part series patches to belong to the same thread, UNLESS + // the candidate series is incomplete, in which case we allow merging complementary parts. + let thread_compatible = + same_thread || is_singleton || (existing_received < existing_total); if author_or_series_match && (!strict_author || (date - existing_date).abs() < 86400) @@ -1851,20 +1876,28 @@ impl Database { && thread_compatible && !index_collision { - matches.push((id, existing_subject_index)); + matches.push((id, existing_subject_index, existing_thread_id)); } } if !matches.is_empty() { - // Sort matches to pick the "best" one to keep (e.g. oldest ID or one with lowest subject index) - // Let's keep the one with the lowest ID (created first) - matches.sort_by_key(|k| k.0); + // Sort matches: Prioritize matches in the same thread over cross-thread matches, then pick by oldest ID. + matches.sort_by_key(|k| { + let same = k.2 == Some(thread_id); + (!same, k.0) + }); let target_id = matches[0].0; + let target_thread_id = matches[0].2; let mut current_subject_index = matches[0].1; - // If we have multiple matches, merge others into target_id - for (merge_from_id, merge_subject_index) in matches.iter().skip(1) { + // If we have multiple matches, merge others into target_id. + // Cross-thread merging of existing distinct patchsets is prevented to avoid accidental stealing. + for (merge_from_id, merge_subject_index, merge_from_thread_id) in matches.iter().skip(1) + { + if *merge_from_thread_id != target_thread_id { + continue; + } let merge_from_id = *merge_from_id; info!("Merging patchset {} into {}", merge_from_id, target_id); @@ -4955,7 +4988,7 @@ mod tests { } #[tokio::test] - async fn test_cross_thread_no_merge() { + async fn test_cross_thread_complementary_merge_in_db() { let db = setup_db().await; // 1. Create Thread A and Patchset A (1/2) @@ -5045,10 +5078,10 @@ mod tests { .unwrap() .unwrap(); - // 3. Assert they DID NOT merge (ps2 should NOT equal ps1) - assert_ne!( + // 3. Assert they DID merge (ps2 should equal ps1) + assert_eq!( ps1, ps2, - "Patchsets from different threads should NOT merge even if author/time match" + "Patchsets from different threads SHOULD merge when they are complementary and incomplete" ); // 4. Verify total patches count or received parts @@ -5060,13 +5093,13 @@ mod tests { .await .unwrap() .unwrap(); - assert_eq!(details1["received_parts"], 1); + assert_eq!(details1["received_parts"], 2); let details2 = db .get_patchset_details(ps2, None, None) .await .unwrap() .unwrap(); - assert_eq!(details2["received_parts"], 1); + assert_eq!(details2["received_parts"], 2); } #[tokio::test] diff --git a/tests/cover_letter_version_merge_test.rs b/tests/cover_letter_version_merge_test.rs new file mode 100644 index 000000000..e5196aedb --- /dev/null +++ b/tests/cover_letter_version_merge_test.rs @@ -0,0 +1,169 @@ +// Copyright 2026 The Sashiko Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use sashiko::db::Database; +use sashiko::settings::DatabaseSettings; +use std::sync::Arc; + +async fn setup_db() -> Arc { + let settings = DatabaseSettings { + url: ":memory:".to_string(), + token: String::new(), + }; + let db = Database::new(&settings).await.unwrap(); + db.migrate().await.unwrap(); + Arc::new(db) +} + +#[tokio::test] +async fn test_cover_letter_matching_with_different_versions_should_not_merge() { + let db = setup_db().await; + + // Create Thread + let t1 = db + .create_thread("root1", "Test Series", 1000) + .await + .unwrap(); + + // v1 Series: Part 1/2 + db.create_message( + /* message_id: */ "v1_patch1", + /* thread_id: */ t1, + /* in_reply_to: */ None, + /* author: */ "Author", + /* subject: */ "[PATCH 1/2] Fix bug", + /* date: */ 1000, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1 = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ None, // No cover letter yet + /* message_id: */ "v1_patch1", + /* subject: */ "[PATCH 1/2] Fix bug", + /* author: */ "Author", + /* date: */ 1000, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + db.create_patch(ps1, "v1_patch1", 1, "diff").await.unwrap(); + + // v1 Series: Part 2/2 (reply to v1 1/2) + db.create_message( + /* message_id: */ "v1_patch2", + /* thread_id: */ t1, + /* in_reply_to: */ Some("v1_patch1"), + /* author: */ "Author", + /* subject: */ "[PATCH 2/2] Fix bug", + /* date: */ 1010, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1_b = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ + Some("v1_patch1"), // Treated as cover letter by in_reply_to logic + /* message_id: */ "v1_patch2", + /* subject: */ "[PATCH 2/2] Fix bug", + /* author: */ "Author", + /* date: */ 1010, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 2, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + assert_eq!(ps1, ps1_b); // They merge fine + + // v2 Series: Part 1/2 (in reply to v1 1/2) + db.create_message( + /* message_id: */ "v2_patch1", + /* thread_id: */ t1, + /* in_reply_to: */ Some("v1_patch1"), + /* author: */ "Author", + /* subject: */ "[PATCH v2 1/2] Fix bug", + /* date: */ 1100, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps2 = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ + Some("v1_patch1"), // Incoming patch has in_reply_to pointing to v1 + /* message_id: */ "v2_patch1", + /* subject: */ "[PATCH v2 1/2] Fix bug", + /* author: */ "Author", + /* date: */ 1100, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ Some(2), + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + assert_ne!( + ps1, ps2, + "v2 patch 1/2 should NOT merge into v1 patchset via cover_letter_message_id matching" + ); +} diff --git a/tests/cross_thread_complementary_merge_test.rs b/tests/cross_thread_complementary_merge_test.rs new file mode 100644 index 000000000..ca6fc5f2d --- /dev/null +++ b/tests/cross_thread_complementary_merge_test.rs @@ -0,0 +1,131 @@ +// Copyright 2026 The Sashiko Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use sashiko::db::Database; +use sashiko::settings::DatabaseSettings; +use std::sync::Arc; + +async fn setup_db() -> Arc { + let settings = DatabaseSettings { + url: ":memory:".to_string(), + token: String::new(), + }; + let db = Database::new(&settings).await.unwrap(); + db.migrate().await.unwrap(); + Arc::new(db) +} + +#[tokio::test] +async fn test_cross_thread_complementary_merge() { + let db = setup_db().await; + + // 1. Create Thread A + let t_a = db + .create_thread("root_a", "Series Subject", 1000) + .await + .unwrap(); + + // First batch: patches 1 to 40 out of 60 + db.create_message( + /* message_id: */ "patch_1", + /* thread_id: */ t_a, + /* in_reply_to: */ None, + /* author: */ "Author", + /* subject: */ "[PATCH 1/60] Fix bug part 1", + /* date: */ 1000, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1 = db + .create_patchset( + /* thread_id: */ t_a, + /* cover_letter_message_id: */ None, + /* message_id: */ "patch_1", + /* subject: */ "[PATCH 1/60] Fix bug part 1", + /* author: */ "Author", + /* date: */ 1000, + /* total_parts: */ 60, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + db.create_patch(ps1, "patch_1", 1, "diff").await.unwrap(); + + // 2. Resend second batch (41 to 60 out of 60) in Thread B + let t_b = db + .create_thread("root_b", "Series Subject", 1010) + .await + .unwrap(); + + db.create_message( + /* message_id: */ "patch_41", + /* thread_id: */ t_b, + /* in_reply_to: */ None, // No connection to Thread A + /* author: */ "Author", + /* subject: */ "[PATCH 41/60] Fix bug part 41", + /* date: */ 1010, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps2 = db + .create_patchset( + /* thread_id: */ t_b, + /* cover_letter_message_id: */ None, + /* message_id: */ "patch_41", + /* subject: */ "[PATCH 41/60] Fix bug part 41", + /* author: */ "Author", + /* date: */ 1010, + /* total_parts: */ 60, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 41, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + // 3. Assert that they DO merge (ps1 == ps2) + assert_eq!( + ps1, ps2, + "Patchsets from different threads should merge when they are complementary parts of the same series." + ); +} diff --git a/tests/merge_bug_prefixes.rs b/tests/merge_bug_prefixes.rs index d2f79d83d..60f6fbfc5 100644 --- a/tests/merge_bug_prefixes.rs +++ b/tests/merge_bug_prefixes.rs @@ -99,7 +99,7 @@ async fn test_merge_prefixes_mismatch_should_split() { } #[tokio::test] -async fn test_merge_prefixes_match_should_not_merge() { +async fn test_merge_prefixes_match_should_merge_if_complementary() { let db = setup_db().await; let t1 = db @@ -161,8 +161,8 @@ async fn test_merge_prefixes_match_should_not_merge() { .unwrap() .unwrap(); - assert_ne!( + assert_eq!( ps1, ps2, - "Matching prefixes (net-next) should NOT merge across threads" + "Matching prefixes (net-next) SHOULD merge across threads when they are complementary" ); }