From ed073de1bfc5123cd8189886f69b4f2de12d3497 Mon Sep 17 00:00:00 2001 From: Stanislau Hlebik Date: Tue, 16 Nov 2021 11:15:53 -0800 Subject: [PATCH] mononoke: make version mapping non-optional in WorkingCopyEquivalence Summary: In the next diff we are going to refactor CommitSyncOutcome::NotSyncCandidate to have a version, which would mean that the version should always be present even for NotSyncCandidate So in this diff let's make mapping version non-optional. Note that at the moment our prod tables have some of the mapping entries with a non-empty version - I'm going to fix it before landing this diff. Reviewed By: mitrandir77 Differential Revision: D32428740 fbshipit-source-id: be9f8b104a14497bf5a524c68c0fd53beba22ee1 --- .../commit_rewriting/backsyncer/src/tests.rs | 6 ++-- .../src/commit_sync_outcome.rs | 16 ++-------- .../synced_commit_mapping/src/lib.rs | 29 +++++++++++-------- .../synced_commit_mapping/test/main.rs | 13 ++------- 4 files changed, 26 insertions(+), 38 deletions(-) diff --git a/eden/mononoke/commit_rewriting/backsyncer/src/tests.rs b/eden/mononoke/commit_rewriting/backsyncer/src/tests.rs index a74162a6c4..06f591dc6a 100644 --- a/eden/mononoke/commit_rewriting/backsyncer/src/tests.rs +++ b/eden/mononoke/commit_rewriting/backsyncer/src/tests.rs @@ -1856,6 +1856,7 @@ async fn preserve_premerge_commit( bcs_id ); + let version = CommitSyncConfigVersion("noop".to_string()); // Doesn't matter what mover to use - we are going to preserve the commit anyway let small_to_large_sync_config = { let repos = CommitSyncRepos::SmallToLarge { @@ -1868,7 +1869,6 @@ async fn preserve_premerge_commit( let bookmark_renamer_type = BookmarkRenamerType::Noop; let mover_type = MoverType::Noop; - let version = CommitSyncConfigVersion("noop".to_string()); let version_config = CommitSyncConfig { large_repo_id: large_repo.get_repoid(), common_pushrebase_bookmarks: vec![BookmarkName::new("master")?], @@ -1879,7 +1879,7 @@ async fn preserve_premerge_commit( }; lv_cfg_src.add_config(version_config); - lv_cfg_src.add_current_version(version); + lv_cfg_src.add_current_version(version.clone()); let common = bookmark_renamer_type .get_common_repo_config(small_repo.get_repoid(), large_repo.get_repoid()); lv_cfg_src.add_common_config(common); @@ -1907,7 +1907,7 @@ async fn preserve_premerge_commit( large_bcs_id: bcs_id, small_repo_id: another_repo_id, small_bcs_id: None, - version_name: None, + version_name: Some(version.clone()), }, ) .await?; diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_sync_outcome.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_sync_outcome.rs index c1aee6c716..68066185f4 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_sync_outcome.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/commit_sync_outcome.rs @@ -227,19 +227,9 @@ pub async fn get_plural_commit_sync_outcome<'a, M: SyncedCommitMapping>( Some(WorkingCopyEquivalence::NoWorkingCopy(_version)) => { Ok(Some(PluralCommitSyncOutcome::NotSyncCandidate)) } - Some(WorkingCopyEquivalence::WorkingCopy(cs_id, maybe_version)) => { - let version = maybe_version.ok_or_else(|| { - anyhow!( - "no sync commit version specified for equivalent working copy {} -> {} (source repo {}, target repo {})", - source_cs_id.0, cs_id, - source_repo_id, - target_repo_id, - ) - })?; - Ok(Some( - PluralCommitSyncOutcome::EquivalentWorkingCopyAncestor(cs_id, version), - )) - } + Some(WorkingCopyEquivalence::WorkingCopy(cs_id, version)) => Ok(Some( + PluralCommitSyncOutcome::EquivalentWorkingCopyAncestor(cs_id, version), + )), } } diff --git a/eden/mononoke/commit_rewriting/synced_commit_mapping/src/lib.rs b/eden/mononoke/commit_rewriting/synced_commit_mapping/src/lib.rs index e137f182f8..12aacb7f51 100644 --- a/eden/mononoke/commit_rewriting/synced_commit_mapping/src/lib.rs +++ b/eden/mononoke/commit_rewriting/synced_commit_mapping/src/lib.rs @@ -11,7 +11,7 @@ use sql::{Connection, Transaction}; use sql_construct::{SqlConstruct, SqlConstructFromMetadataDatabaseConfig}; use sql_ext::SqlConnections; -use anyhow::Error; +use anyhow::{anyhow, Error}; use async_trait::async_trait; use auto_impl::auto_impl; use context::{CoreContext, PerfCounterType}; @@ -163,9 +163,9 @@ pub struct EquivalentWorkingCopyEntry { pub enum WorkingCopyEquivalence { /// There's no matching working copy. It can happen if a pre-big-merge commit from one small /// repo is mapped into another small repo - NoWorkingCopy(Option), + NoWorkingCopy(CommitSyncConfigVersion), /// ChangesetId of matching working copy and CommitSyncConfigVersion that was used for mapping - WorkingCopy(ChangesetId, Option), + WorkingCopy(ChangesetId, CommitSyncConfigVersion), } #[async_trait] @@ -515,6 +515,7 @@ impl SyncedCommitMapping for SqlSyncedCommitMapping { WorkingCopy(wc, mapping) => (Some(wc), mapping), NoWorkingCopy(mapping) => (None, mapping), }; + let expected_version = Some(expected_version); if (expected_bcs_id != small_bcs_id) || (expected_version != version_name) { let err = ErrorKind::InconsistentWorkingCopyEntry { expected_bcs_id, @@ -573,18 +574,22 @@ impl SyncedCommitMapping for SqlSyncedCommitMapping { maybe_mapping, ) = row; + let mapping = maybe_mapping.ok_or_else(|| { + anyhow!( + "unexpected empty mapping for {}, {}->{}", + source_bcs_id, + source_repo_id, + target_repo_id + ) + })?; if target_repo_id == large_repo_id { - Some(WorkingCopyEquivalence::WorkingCopy( - large_bcs_id, - maybe_mapping, - )) + Some(WorkingCopyEquivalence::WorkingCopy(large_bcs_id, mapping)) } else { match maybe_small_bcs_id { - Some(small_bcs_id) => Some(WorkingCopyEquivalence::WorkingCopy( - small_bcs_id, - maybe_mapping, - )), - None => Some(WorkingCopyEquivalence::NoWorkingCopy(maybe_mapping)), + Some(small_bcs_id) => { + Some(WorkingCopyEquivalence::WorkingCopy(small_bcs_id, mapping)) + } + None => Some(WorkingCopyEquivalence::NoWorkingCopy(mapping)), } } } diff --git a/eden/mononoke/commit_rewriting/synced_commit_mapping/test/main.rs b/eden/mononoke/commit_rewriting/synced_commit_mapping/test/main.rs index 271aff50e5..a1b9c7ee9b 100644 --- a/eden/mononoke/commit_rewriting/synced_commit_mapping/test/main.rs +++ b/eden/mononoke/commit_rewriting/synced_commit_mapping/test/main.rs @@ -57,7 +57,7 @@ async fn add_and_get(fb: FacebookInit, mapping: M) { res, Some(WorkingCopyEquivalence::WorkingCopy( bonsai::TWOS_CSID, - Some(version_name.clone()), + version_name.clone(), )) ); @@ -87,7 +87,7 @@ async fn add_and_get(fb: FacebookInit, mapping: M) { res, Some(WorkingCopyEquivalence::WorkingCopy( bonsai::FOURS_CSID, - Some(version_name.clone()), + version_name.clone(), )) ); @@ -169,7 +169,7 @@ async fn equivalent_working_copy(fb: FacebookInit, mappi res, Some(WorkingCopyEquivalence::WorkingCopy( bonsai::TWOS_CSID, - Some(version_name) + version_name )) ); @@ -187,13 +187,6 @@ async fn equivalent_working_copy(fb: FacebookInit, mappi .expect("Failed to insert working copy"); assert_eq!(result, true); - let res = mapping - .get_equivalent_working_copy(&ctx, REPO_ZERO, bonsai::THREES_CSID, REPO_ONE) - .await - .expect("get equivalent wc failed, should succeed"); - - assert_eq!(res, Some(WorkingCopyEquivalence::NoWorkingCopy(None))); - let should_fail = EquivalentWorkingCopyEntry { large_repo_id: REPO_ZERO, large_bcs_id: bonsai::THREES_CSID,