diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/Cargo.toml b/eden/mononoke/commit_rewriting/cross_repo_sync/Cargo.toml index 10c6644e25..d668a8d5fb 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/Cargo.toml +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/Cargo.toml @@ -21,6 +21,7 @@ blobsync = { path = "../../blobrepo/blobsync" } bookmark_renaming = { path = "../bookmark_renaming" } bookmarks = { path = "../../bookmarks" } context = { path = "../../server/context" } +iterhelpers = { path = "../../common/iterhelpers" } live_commit_sync_config = { path = "../live_commit_sync_config" } manifest = { path = "../../manifest" } mercurial_types = { path = "../../mercurial/types" } diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/lib.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/lib.rs index 0de573ec28..b74f052307 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/src/lib.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/lib.rs @@ -8,7 +8,7 @@ #![deny(warnings)] #![feature(trait_alias)] -use anyhow::{bail, format_err, Error}; +use anyhow::{bail, format_err, Context, Error}; use blobrepo::{save_bonsai_changesets, BlobRepo}; use blobrepo_hg::BlobRepoHg; use blobstore::Loadable; @@ -37,9 +37,9 @@ use movers::Mover; use pushrebase::{do_pushrebase_bonsai, PushrebaseError}; use reachabilityindex::LeastCommonAncestorsHint; use slog::{debug, info}; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, VecDeque}; +use std::fmt; use std::sync::Arc; -use std::{collections::VecDeque, fmt}; use synced_commit_mapping::{ EquivalentWorkingCopyEntry, SyncedCommitMapping, SyncedCommitMappingEntry, }; @@ -47,12 +47,14 @@ use thiserror::Error; use topo_sort::sort_topological; pub use commit_syncer_args::CommitSyncerArgs; +use merge_utils::get_version_for_merge; use pushrebase_hook::CrossRepoSyncPushrebaseHook; use types::{Source, Target}; mod commit_sync_data_provider; pub mod commit_sync_outcome; mod commit_syncer_args; +mod merge_utils; mod pushrebase_hook; pub mod types; pub mod validation; @@ -1176,6 +1178,33 @@ where } } + + /// Get `CommitSyncConfigVersion` to use while remapping a + /// merge commit (`source_cs_id`) + /// The idea is to derive this version from the `parent_outcomes` + /// according to the following rules: + /// - all `NotSyncCandidate` parents are ignored + /// - `Preserved` parents are prohibited (sync fails), and it + /// is expected that such cases are handled by the caller in a + /// separate flow + /// - all `RewrittenAs` and `EquivalentWorkingCopyAncestor` + /// parents have the same (non-None) version associated + async fn get_mover_to_use_for_merge<'a>( + &'a self, + ctx: &CoreContext, + source_cs_id: ChangesetId, + parent_outcomes: impl IntoIterator, + ) -> Result<(Mover, CommitSyncConfigVersion), Error> { + let version = get_version_for_merge( + source_cs_id, + parent_outcomes, + self.get_current_version(ctx)?, + )?; + + let mover = self.get_mover_by_version(&version)?; + Ok((mover, version)) + } + // See more details about the algorithm in https://fb.quip.com/s8fYAOxEohtJ // A few important notes: // 1) Merges are synced only in LARGE -> SMALL direction. @@ -1193,7 +1222,6 @@ where let source_cs_id = cs.get_changeset_id(); let cs = cs.into_mut(); - let (_, _, rewrite_paths) = self.get_source_target_mover(&ctx)?; let parent_outcomes = stream::iter(cs.parents.clone().into_iter().map(|p| { self.get_commit_sync_outcome(ctx.clone(), p) @@ -1247,7 +1275,7 @@ where return Ok(None); } - // At this point we know that there's at least parent after big merge. However we still + // At this point we know that there's at least one parent after big merge. However we still // might have a parent that's NotSyncCandidate // // B @@ -1276,21 +1304,36 @@ where } }) .collect(); + let cs = self.strip_removed_parents(cs, new_parents.keys().collect())?; if !new_parents.is_empty() { + let (mover, version) = self + .get_mover_to_use_for_merge( + &ctx, + source_cs_id, + sync_outcomes.iter().map(|(_, outcome)| outcome), + ) + .await + .context("failed getting a mover to use for merge rewriting")?; + match rewrite_commit( ctx.clone(), cs, &new_parents, - rewrite_paths, + mover, self.get_source_repo().clone(), ) .await? { Some(rewritten) => { let target_cs_id = self - .upload_rewritten_and_update_mapping(ctx.clone(), source_cs_id, rewritten) + .upload_rewritten_and_update_mapping( + ctx.clone(), + source_cs_id, + rewritten, + version, + ) .await?; Ok(Some(target_cs_id)) } @@ -1321,6 +1364,7 @@ where ctx: CoreContext, source_cs_id: ChangesetId, rewritten: BonsaiChangesetMut, + version: CommitSyncConfigVersion, ) -> Result { let (source_repo, target_repo, _) = self.get_source_target_mover(&ctx)?; @@ -1336,10 +1380,11 @@ where // update_mapping also updates working copy equivalence, so no need // to do it separately - update_mapping( + update_mapping_with_version( ctx.clone(), hashmap! { source_cs_id => target_cs_id}, &self, + &version, ) .await?; return Ok(target_cs_id); diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/merge_utils.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/merge_utils.rs new file mode 100644 index 0000000000..8b2a286fbd --- /dev/null +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/merge_utils.rs @@ -0,0 +1,187 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This software may be used and distributed according to the terms of the + * GNU General Public License version 2. + */ + +use anyhow::{bail, format_err, Error}; +use iterhelpers::get_only_item; +use metaconfig_types::CommitSyncConfigVersion; +use mononoke_types::ChangesetId; +use std::collections::HashSet; + +use crate::commit_sync_outcome::CommitSyncOutcome; + +/// For merge commit `source_cs_is` and `parent_outcomes` for +/// its parents, get the version to use to construct a mover +pub fn get_version_for_merge<'a>( + source_cs_id: ChangesetId, + parent_outcomes: impl IntoIterator, + current_version: CommitSyncConfigVersion, +) -> Result { + let unique_versions = { + let mut versions = HashSet::new(); + for parent_outcome in parent_outcomes.into_iter() { + use CommitSyncOutcome::*; + match parent_outcome { + NotSyncCandidate => continue, + Preserved => { + bail!("cannot syncs merges of rewritten and preserved commits"); + } + RewrittenAs(_, None) | EquivalentWorkingCopyAncestor(_, None) => { + // TODO: in the future we need to bail here, for now let's + // do a hacky thing and get a current version + versions.insert(current_version.clone()); + } + RewrittenAs(_, Some(version)) | EquivalentWorkingCopyAncestor(_, Some(version)) => { + versions.insert(version.clone()); + } + } + } + + versions + }; + + let version_res: Result<_, Error> = get_only_item( + unique_versions, + || { + format_err!( + "unexpected absence of rewritten parents for {}", + source_cs_id, + ) + }, + |v1, v2| { + format_err!( + "too many CommitSyncConfig versions used to remap parents of {}: {}, {} (may be more)", + source_cs_id, + v1, + v2, + ) + }, + ); + // Type inference cannot figure the error type on its own + let version = version_res?; + + Ok(version) +} + +#[cfg(test)] +mod tests { + use super::*; + use fbinit::FacebookInit; + use mononoke_types_mocks::changesetid as bonsai; + + #[fbinit::test] + fn test_merge_version_determinator_success_single_rewritten(_fb: FacebookInit) { + // Basic test: there's a single non-preserved parent, determining + // Mover version should succeed + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let v2 = CommitSyncConfigVersion("TEST_VERSION_2".to_string()); + let parent_outcomes = [ + NotSyncCandidate, + RewrittenAs(bonsai::FOURS_CSID, Some(v1.clone())), + ]; + + let rv = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v2).unwrap(); + assert_eq!(rv, v1); + } + + #[fbinit::test] + fn test_merge_version_determinator_success(_fb: FacebookInit) { + // There are two rewritten parents, both preserved with the same + // version. Determining Mover version should succeed + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let v2 = CommitSyncConfigVersion("TEST_VERSION_2".to_string()); + let parent_outcomes = [ + RewrittenAs(bonsai::FOURS_CSID, Some(v1.clone())), + RewrittenAs(bonsai::THREES_CSID, Some(v1.clone())), + ]; + + let rv = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v2).unwrap(); + assert_eq!(rv, v1); + } + + #[fbinit::test] + fn test_merge_version_determinator_failure_different_versions(_fb: FacebookInit) { + // There are two rewritten parents, with different versions + // Determining Mover version should fail + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let v2 = CommitSyncConfigVersion("TEST_VERSION_2".to_string()); + let parent_outcomes = [ + RewrittenAs(bonsai::FOURS_CSID, Some(v1)), + RewrittenAs(bonsai::THREES_CSID, Some(v2.clone())), + ]; + + let e = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v2).unwrap_err(); + assert!( + format!("{}", e).contains("too many CommitSyncConfig versions used to remap parents") + ); + } + + #[fbinit::test] + fn test_merge_version_determinator_failure_current_version(_fb: FacebookInit) { + // There are two rewritten parents, one with a version, another without + // Our hack uses current version as filling for a missing one, and current version + // does not equal the version used on the other parent. Determining + // Mover version should fail. + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let v2 = CommitSyncConfigVersion("TEST_VERSION_2".to_string()); + let parent_outcomes = [ + RewrittenAs(bonsai::FOURS_CSID, Some(v1)), + RewrittenAs(bonsai::THREES_CSID, None), + ]; + + let e = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v2).unwrap_err(); + assert!( + format!("{}", e).contains("too many CommitSyncConfig versions used to remap parents") + ); + } + + #[fbinit::test] + fn test_merge_version_determinator_success_current_version(_fb: FacebookInit) { + // There are two rewritten parents, one with a version, another without + // Our hack uses current version as filling for a missing one, and current version + // equals the version used on the other parent. Determining + // Mover version should succeed. + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let parent_outcomes = [ + RewrittenAs(bonsai::FOURS_CSID, Some(v1.clone())), + RewrittenAs(bonsai::THREES_CSID, None), + ]; + + let rv = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v1.clone()).unwrap(); + assert_eq!(rv, v1); + } + + #[fbinit::test] + fn test_merge_version_determinator_success_current_version_2(_fb: FacebookInit) { + // Both rewritten parents are missing a version. Our hack uses current + // version as a fill, so determining Mover version should succeed + use CommitSyncOutcome::*; + let v1 = CommitSyncConfigVersion("TEST_VERSION_1".to_string()); + let parent_outcomes = [ + RewrittenAs(bonsai::FOURS_CSID, None), + RewrittenAs(bonsai::THREES_CSID, None), + ]; + + let rv = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v1.clone()).unwrap(); + assert_eq!(rv, v1); + } + + #[fbinit::test] + fn test_merge_version_determinator_failure_all_not_candidates(_fb: FacebookInit) { + // All parents are preserved, this function should not have been called + use CommitSyncOutcome::*; + let v2 = CommitSyncConfigVersion("TEST_VERSION_2".to_string()); + let parent_outcomes = [NotSyncCandidate, NotSyncCandidate]; + + let e = get_version_for_merge(bonsai::ONES_CSID, &parent_outcomes, v2).unwrap_err(); + assert!(format!("{}", e).contains("unexpected absence of rewritten parents")); + } +}