Unit test for replacing a file with a submodule

Summary:
## This stack
See D55064943.

## This diff
Adds a unit test for a specific edge case, where a submodule is added to the small repo (repo A) in the same path of a regular file.

**Expected behaviour:** file is deleted in repo A's folder in the large repo and the new submodule is added as a directory containing its expansion.

**Current behaviour:** This triggered a couple of bugs, which will be fixed in the next diffs:
1. We try to read the regular file as a submodule file, to get the git hash and get the previous submodule pointer. This crashes because the file doesn't contain a git hash.
2. We don't generate the deletion of the regular file in the large repo bonsai. This leads to failures in the derivation of fsnodes.

This brings another important test: derive all data types in all tests to make sure we don't break anything in the large repo, so I'm adding a TODO for this.

## Notes
- To add this test, I extended the helpers to allow passing more repos to be used a submodule deps in that test case.
- I left some code commented that represents what the final unit test should look like once these issues are fixed, which will happen soon in the next few diffs.

Reviewed By: mitrandir77

Differential Revision: D55068950

fbshipit-source-id: b4f2331852c7866894513186a919fc56e9900f3f

eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules_test_utils.rs

@@ -141,6 +141,8 @@ impl ExpectedChangeset {
 /// large repo and all the commit syncer with a config that expands submodules.
 pub(crate) async fn build_submodule_sync_test_data(
     fb: FacebookInit,
+    // Add more small repo submodule dependencies for the test case
+    extra_submodule_deps: Vec<(NonRootMPath, TestRepo)>,
 ) -> Result<SubmoduleSyncTestData> {
     let ctx = CoreContext::test_mock(fb.clone());
     let (small_repo, large_repo, mapping, live_commit_sync_config, test_sync_config_source) =
@@ -180,6 +182,7 @@ pub(crate) async fn build_submodule_sync_test_data(
         mapping.clone(),
         live_commit_sync_config.clone(),
         test_sync_config_source,
+        extra_submodule_deps,
     )?;
 
     rebase_root_on_master(ctx.clone(), &commit_syncer, *repo_a_root).await?;
@@ -256,17 +259,37 @@ pub(crate) async fn build_repo_b(
       # bookmark: B_B master
   "#;
 
-    let repo = build_mononoke_git_mirror_repo(fb).await?;
+    let repo = build_mononoke_git_mirror_repo(fb, "repo_b").await?;
     let (cs_map, _) = extend_from_dag_with_actions(&ctx, &repo, DAG).await?;
 
     Ok((repo, cs_map))
 }
 
-async fn build_mononoke_git_mirror_repo(fb: FacebookInit) -> Result<TestRepo> {
+/// Builds repo C, which will be used as a submodule dependency of repo A.
+pub(crate) async fn build_repo_c(
+    fb: FacebookInit,
+) -> Result<(TestRepo, BTreeMap<String, ChangesetId>)> {
+    let ctx = CoreContext::test_mock(fb);
+
+    const DAG: &str = r#"
+    C_A-C_B
+
+    # message: C_A "[C] first commit in submodule C"
+    # message: C_B "[C] second commit in submodule C"
+    # bookmark: C_B master
+"#;
+
+    let repo = build_mononoke_git_mirror_repo(fb, "repo_c").await?;
+    let (cs_map, _) = extend_from_dag_with_actions(&ctx, &repo, DAG).await?;
+
+    Ok((repo, cs_map))
+}
+
+async fn build_mononoke_git_mirror_repo(fb: FacebookInit, repo_name: &str) -> Result<TestRepo> {
     let available_configs = derived_data_available_config();
 
     let repo = TestRepoFactory::new(fb)?
-        .with_name("repo_b")
+        .with_name(repo_name)
         .with_config_override(|cfg| {
             cfg.derived_data_config.available_configs = available_configs;
 
@@ -289,6 +312,7 @@ pub(crate) fn create_repo_a_to_large_repo_commit_syncer(
     mapping: SqlSyncedCommitMapping,
     live_commit_sync_config: Arc<dyn LiveCommitSyncConfig>,
     test_sync_config_source: TestLiveCommitSyncConfigSource,
+    extra_submodule_deps: Vec<(NonRootMPath, TestRepo)>,
 ) -> Result<CommitSyncer<SqlSyncedCommitMapping, TestRepo>, Error> {
     let small_repo_id = small_repo.repo_identity().id();
     let large_repo_id = large_repo.repo_identity().id();
@@ -298,6 +322,7 @@ pub(crate) fn create_repo_a_to_large_repo_commit_syncer(
         small_repo_id,
         repo_b.repo_identity().id(),
         prefix,
+        extra_submodule_deps.clone(),
     )?;
 
     let common_config = CommonCommitSyncConfig {
@@ -310,9 +335,12 @@ pub(crate) fn create_repo_a_to_large_repo_commit_syncer(
         },
         large_repo_id: large_repo.repo_identity().id(),
     };
-    let submodule_deps = SubmoduleDeps::ForSync(
-        hashmap! {NonRootMPath::new("submodules/repo_b")? => repo_b.clone()},
-    );
+
+    let mut all_submodule_deps = extra_submodule_deps.into_iter().collect::<HashMap<_, _>>();
+
+    all_submodule_deps.insert(NonRootMPath::new("submodules/repo_b")?, repo_b.clone());
+    let submodule_deps = SubmoduleDeps::ForSync(all_submodule_deps);
+
     let repos = CommitSyncRepos::new(small_repo, large_repo, submodule_deps, &common_config)?;
 
     test_sync_config_source.add_config(commit_sync_config);
@@ -335,10 +363,18 @@ pub(crate) fn create_base_commit_sync_config(
     repo_a_id: RepositoryId,
     repo_b_id: RepositoryId,
     prefix: &str,
+    extra_submodule_deps: Vec<(NonRootMPath, TestRepo)>,
 ) -> Result<CommitSyncConfig, Error> {
+    let mut submodule_deps = extra_submodule_deps
+        .into_iter()
+        .map(|(path, repo)| (path, repo.repo_identity().id()))
+        .collect::<HashMap<_, _>>();
+
+    submodule_deps.insert(NonRootMPath::new("submodules/repo_b")?, repo_b_id);
+
     let small_repo_submodule_config = SmallRepoGitSubmoduleConfig {
         git_submodules_action: GitSubmodulesChangesAction::Expand,
-        submodule_dependencies: hashmap! {NonRootMPath::new("submodules/repo_b")? => repo_b_id},
+        submodule_dependencies: submodule_deps,
         ..Default::default()
     };
     let small_repo_config = SmallRepoCommitSyncConfig {
@@ -364,6 +400,7 @@ pub(crate) fn create_base_commit_sync_config(
 
 /// Derived data types that should be enabled in all test repos
 pub(crate) fn derived_data_available_config() -> HashMap<String, DerivedDataTypesConfig> {
+    // TODO(T181473986): enable and derive all types in the large repo
     let derived_data_types_config = DerivedDataTypesConfig {
         types: hashset! {
             ChangesetInfo::VARIANT,

eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules_tests.rs

@@ -47,7 +47,7 @@ async fn test_submodule_expansion_basic(fb: FacebookInit) -> Result<()> {
         large_repo,
         commit_syncer,
         ..
-    } = build_submodule_sync_test_data(fb).await?;
+    } = build_submodule_sync_test_data(fb, vec![]).await?;
 
     // Check mappings from base commits
     check_mapping(
@@ -166,7 +166,7 @@ async fn test_submodule_deletion(fb: FacebookInit) -> Result<()> {
         large_repo,
         commit_syncer,
         ..
-    } = build_submodule_sync_test_data(fb).await?;
+    } = build_submodule_sync_test_data(fb, vec![]).await?;
 
     const MESSAGE: &str = "Delete repo_b submodule in repo_a";
     let cs_id = CreateCommitContext::new(&ctx, &repo_a, vec![*repo_a_cs_map.get("A_C").unwrap()])
@@ -234,7 +234,7 @@ async fn test_implicitly_deleting_submodule(fb: FacebookInit) -> Result<()> {
         large_repo,
         commit_syncer,
         ..
-    } = build_submodule_sync_test_data(fb).await?;
+    } = build_submodule_sync_test_data(fb, vec![]).await?;
 
     const MESSAGE: &str = "Implicitly delete repo_b submodule in repo_a";
 
@@ -292,7 +292,7 @@ async fn test_implicit_deletions_inside_submodule_repo(fb: FacebookInit) -> Resu
         large_repo,
         commit_syncer,
         ..
-    } = build_submodule_sync_test_data(fb).await?;
+    } = build_submodule_sync_test_data(fb, vec![]).await?;
 
     // Create a directory with 2 files in repo B
     let add_directory_in_repo_b =
@@ -371,8 +371,106 @@ async fn test_implicit_deletions_inside_submodule_repo(fb: FacebookInit) -> Resu
     Ok(())
 }
 
-// TODO(T179534458): test implicitly deleting file by adding submodule in same
-// path
+/// Test adding a submodule dependency in the source repo in the path of an existing
+/// file. This should generate a deletion of the file in the large repo, along
+/// with the expansion of the submodule.
+#[fbinit::test]
+async fn test_implicitly_deleting_file_with_submodule(fb: FacebookInit) -> Result<()> {
+    let ctx = CoreContext::test_mock(fb.clone());
+
+    // Create repo C, to be added as a submodule in repo A.
+    let (repo_c, repo_c_cs_map) = build_repo_c(fb).await?;
+
+    let SubmoduleSyncTestData {
+        repo_a_info: (repo_a, repo_a_cs_map),
+        repo_b_info: (_repo_b, _repo_b_cs_map),
+        large_repo,
+        commit_syncer,
+        ..
+    } = build_submodule_sync_test_data(
+        fb,
+        // Add it as a submdule in the path of an existing file.
+        vec![(NonRootMPath::new("A_A").unwrap(), repo_c.clone())],
+    )
+    .await?;
+
+    let repo_c_mapped_git_commit = repo_c
+        .repo_derived_data()
+        .derive::<MappedGitCommitId>(&ctx, repo_c_cs_map["C_B"])
+        .await?;
+
+    let repo_c_git_commit_hash = *repo_c_mapped_git_commit.oid();
+
+    const MESSAGE: &str = "Add submodule on path of existing file";
+    let cs_id = CreateCommitContext::new(&ctx, &repo_a, vec![repo_a_cs_map["A_C"]])
+        .set_message(MESSAGE)
+        .add_file_with_type(
+            "A_A",
+            repo_c_git_commit_hash.into_inner(),
+            FileType::GitSubmodule,
+        )
+        .commit()
+        .await?;
+
+    println!("Created commit in source repo");
+
+    // TODO(T174902563): fix bug where it looks for submodule file in previous
+    // revision assuming it was already a submodule file.
+    let sync_result = sync_to_master(ctx.clone(), &commit_syncer, cs_id).await;
+    assert!(
+        sync_result.is_err_and(|e| {
+            e.to_string().contains(
+                "Failed to fetch content of submodule A_A file containing the submodule's git commit hash"
+            )
+        })
+    );
+
+    let large_repo_changesets = get_all_changeset_data_from_repo(&ctx, &large_repo).await?;
+
+    let expected_cs_id =
+        ChangesetId::from_str("ea0c6e80fe940e97cc43fd5867ac4e72b51f028b43ccbf23bbb3ac28d26d5b75")
+            .unwrap();
+
+    // TODO(T174902563): fix bug where it looks for submodule file in previous
+    // revision assuming it was already a submodule file.
+    // check_mapping(ctx.clone(), &commit_syncer, cs_id, Some(expected_cs_id)).await;
+    check_mapping(ctx.clone(), &commit_syncer, cs_id, None).await;
+
+    compare_expected_changesets_from_basic_setup(
+        large_repo_changesets,
+        // TODO(T174902563): fix bug where it looks for submodule file in previous
+        // revision assuming it was already a submodule file.
+        vec![],
+        // vec![ExpectedChangeset::new_by_file_change(
+        //     MESSAGE,
+        //     vec![
+        //         "repo_a/.x-repo-submodule-A_A",
+        //         "repo_a/A_A/C_A",
+        //         "repo_a/A_A/C_B",
+        //     ],
+        //     vec![
+        //         // TODO(T174902563): file should be explicitly deleted
+        //         // "repo_a/A_A",
+        //     ],
+        // )],
+    )?;
+
+    // TODO(T174902563): generate deletion for file being replaced by submodule
+    let res = check_submodule_metadata_file_in_large_repo(
+        &ctx,
+        &large_repo,
+        expected_cs_id,
+        NonRootMPath::new("repo_a/.x-repo-submodule-A_A")?,
+        &repo_c_git_commit_hash,
+    )
+    .await;
+
+    assert!(res.is_err());
+    // TODO(T174902563): generate deletion for file being replaced by submodule
+    // assert!(res.is_err_and(|e| e.to_string().contains("failed to derive fsnodes batch")));
+
+    Ok(())
+}
 
 // TODO(T179534458): test implicitly deleting directory by adding submodule in same
 // path