From 7467a4fc2e11680f50ec465a080765cd6b8459cc Mon Sep 17 00:00:00 2001 From: Gustavo Galvao Avena Date: Mon, 29 Apr 2024 23:54:11 -0700 Subject: [PATCH] Allow deletion of submodule metadata file in validation Summary: Fixes the issue described in D56633699. Reviewed By: mitrandir77 Differential Revision: D56690832 fbshipit-source-id: ed61affed8c4d3d6c96ff64071b67c2c793cb8b3 --- .../src/git_submodules/utils.rs | 17 +- .../src/git_submodules/validation.rs | 41 ++- .../test/git_submodules/backsync.rs | 16 +- .../test/git_submodules/forward_sync.rs | 302 +++++++++--------- 4 files changed, 203 insertions(+), 173 deletions(-) diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/utils.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/utils.rs index c851ad90e1..120807041f 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/utils.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/utils.rs @@ -186,6 +186,21 @@ pub(crate) async fn get_submodule_file_content_id( cs_id: ChangesetId, path: &NonRootMPath, ) -> Result> { + content_id_of_file_with_type(ctx, repo, cs_id, path, FileType::GitSubmodule).await +} + +/// Returns the content id of a file at a given path if it was os a specific +/// file type. +pub(crate) async fn content_id_of_file_with_type( + ctx: &CoreContext, + repo: &R, + cs_id: ChangesetId, + path: &NonRootMPath, + expected_file_type: FileType, +) -> Result> +where + R: RepoDerivedDataRef + RepoBlobstoreArc, +{ let fsnode_id = repo .repo_derived_data() .derive::(ctx, cs_id) @@ -197,7 +212,7 @@ pub(crate) async fn get_submodule_file_content_id( .await?; match entry { - Some(Entry::Leaf(file)) if *file.file_type() == FileType::GitSubmodule => { + Some(Entry::Leaf(file)) if *file.file_type() == expected_file_type => { Ok(Some(file.content_id().clone())) } _ => Ok(None), diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/validation.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/validation.rs index 51d677a2f2..ff077e4362 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/validation.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/validation.rs @@ -43,6 +43,7 @@ use slog::debug; use crate::git_submodules::expand::SubmoduleExpansionData; use crate::git_submodules::expand::SubmodulePath; use crate::git_submodules::utils::build_recursive_submodule_deps; +use crate::git_submodules::utils::content_id_of_file_with_type; use crate::git_submodules::utils::get_git_hash_from_submodule_file; use crate::git_submodules::utils::get_x_repo_submodule_metadata_file_path; use crate::git_submodules::utils::git_hash_from_submodule_metadata_file; @@ -163,8 +164,30 @@ async fn validate_submodule_expansion<'a, R: Repo>( let metadata_file_fc = match mb_metadata_file_fc { Some(fc) => fc, None => { + // Check if the submodule metadata file existed in any of the + // parents. If it did, it means that a submodule expansion is + // being modified without properly updating the metadata file. + let submodule_metadata_file_exists = stream::iter(bonsai.parents()) + .map(|cs_id| { + content_id_of_file_with_type( + ctx, + &sm_exp_data.large_repo, + cs_id, + &metadata_file_path, + FileType::Regular, + ) + }) + .buffered(10) + .boxed() + .try_collect::>() + .await? + .into_iter() + // If a content id is returned, the submodule metadata file + // existed in the parent changeset + .any(|mb_content_id| mb_content_id.is_some()); + // This means that the metadata file wasn't modified - if submodule_expansion_changed { + if submodule_expansion_changed && submodule_metadata_file_exists { // Submodule expansion changed, but the metadata file wasn't updated return Err(anyhow!( "Expansion of submodule {submodule_path} changed without updating its metadata file {metadata_file_path}" @@ -181,15 +204,9 @@ async fn validate_submodule_expansion<'a, R: Repo>( FileChange::Change(tfc) => tfc.content_id(), FileChange::UntrackedChange(bfc) => bfc.content_id(), FileChange::Deletion | FileChange::UntrackedDeletion => { - // Metadata file is being deleted, so the entire submodule expansion - // has to deleted as well. - return ensure_submodule_expansion_deletion( - ctx, - sm_exp_data, - bonsai, - synced_submodule_path, - ) - .await; + // TODO(T187241943): ensure that submodule expansion is always + // deleted when the metadata file is deleted during backsyncing. + return Ok(bonsai); } }; @@ -256,6 +273,8 @@ async fn validate_submodule_expansion<'a, R: Repo>( Ok(bonsai) } +// TODO(T187241943): ensure submodule expansion is always deleted when metadata +// file is deleted during backsyncing. /// Ensures that, when the x-repo submodule metadata file was deleted, the /// entire submodule expansion is deleted as well. /// @@ -264,7 +283,7 @@ async fn validate_submodule_expansion<'a, R: Repo>( /// `FileChange::Deletion` for all the files in the expansion. /// 2. Implicitly deleted by adding a file in the path of the expansion /// directory. -async fn ensure_submodule_expansion_deletion<'a, R: Repo>( +async fn _ensure_submodule_expansion_deletion<'a, R: Repo>( ctx: &'a CoreContext, sm_exp_data: SubmoduleExpansionData<'a, R>, // Bonsai from the large repo diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/backsync.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/backsync.rs index 64efa4cd6b..cccb4352db 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/backsync.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/backsync.rs @@ -259,10 +259,11 @@ async fn test_changing_submodule_metadata_pointer_to_git_commit_from_another_rep Ok(()) } -/// Test that manually deleting the submodule metadata file without deleting -/// the expansion fails. +/// Deleting the submodule metadata file without deleting the expansion is a +/// valid scenario, e.g. when users delete a submodule but keep its static copy +/// in the repo as regular files. #[fbinit::test] -async fn test_deleting_submodule_metadata_file_without_expansion_fails_validation( +async fn test_deleting_submodule_metadata_file_without_expansion_passes_validation( fb: FacebookInit, ) -> Result<()> { let ctx = CoreContext::test_mock(fb.clone()); @@ -302,11 +303,10 @@ async fn test_deleting_submodule_metadata_file_without_expansion_fails_validatio ) .await; - println!("Validation result: {0:#?}", &validation_res); - - let expected_err_msg = - "Submodule metadata file is being deleted without removing the entire submodule expansion"; - assert!(validation_res.is_err_and(|e| { e.to_string().contains(expected_err_msg) })); + assert!( + validation_res.is_ok(), + "Validation failed when working copy matches submodule pointer" + ); Ok(()) } diff --git a/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/forward_sync.rs b/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/forward_sync.rs index 3dcb23dde3..871aec60f4 100644 --- a/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/forward_sync.rs +++ b/eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/forward_sync.rs @@ -543,25 +543,25 @@ async fn test_deleting_submodule_but_keeping_directory(fb: FacebookInit) -> Resu let large_repo_cs_id = sync_to_master(ctx.clone(), &commit_syncer, del_md_file_cs_id) .await .context("sync_to_master failed") - .and_then(|res| res.ok_or(anyhow!("No commit was synced"))); + .and_then(|res| res.ok_or(anyhow!("No commit was synced")))?; println!("large_repo_cs_id: {0:#?}", large_repo_cs_id); - // assert_working_copy_matches_expected( - // &ctx, - // &large_repo, - // large_repo_cs_id, - // vec![ - // "large_repo_root", - // "repo_a/A_A", - // "repo_a/A_B", - // "repo_a/A_C", - // // Files from the submodule are now regular files in the small repo - // "repo_a/submodules/repo_b/B_A", - // "repo_a/submodules/repo_b/B_B", - // ], - // ) - // .await?; + assert_working_copy_matches_expected( + &ctx, + &large_repo, + large_repo_cs_id, + vec![ + "large_repo_root", + "repo_a/A_A", + "repo_a/A_B", + "repo_a/A_C", + // Files from the submodule are now regular files in the small repo + "repo_a/submodules/repo_b/B_A", + "repo_a/submodules/repo_b/B_B", + ], + ) + .await?; const CHANGE_SUBMODULE_PATH_MSG: &str = "Change static copy of repo_b"; @@ -584,7 +584,7 @@ async fn test_deleting_submodule_but_keeping_directory(fb: FacebookInit) -> Resu let large_repo_cs_id = sync_to_master(ctx.clone(), &commit_syncer, chg_sm_path_cs_id) .await .context("sync_to_master failed") - .and_then(|res| res.ok_or(anyhow!("No commit was synced"))); + .and_then(|res| res.ok_or(anyhow!("No commit was synced")))?; println!("large_repo_cs_id: {0:#?}", large_repo_cs_id); @@ -593,73 +593,71 @@ async fn test_deleting_submodule_but_keeping_directory(fb: FacebookInit) -> Resu compare_expected_changesets_from_basic_setup( &large_repo_changesets, &[ - // // Changeset that deletes the submodule metadata file - // ExpectedChangeset::new_by_file_change( - // DELETE_METADATA_FILE_MSG, - // // The submodule files are treated as regular file changes - // vec![ - // "repo_a/submodules/repo_b/B_A", - // "repo_a/submodules/repo_b/B_B", - // ], - // // Only submodule metadata file is deleted - // vec!["repo_a/submodules/.x-repo-submodule-repo_b"], - // ), - // // Changeset that modifies files in the submodule path, which is - // // now a static copy of the submodule - // ExpectedChangeset::new_by_file_change( - // CHANGE_SUBMODULE_PATH_MSG, - // // The submodule files are treated as regular file changes - // vec![ - // "repo_a/submodules/repo_b/B_B", - // "repo_a/submodules/repo_b/B_C", - // ], - // // Only submodule metadata file is deleted - // vec!["repo_a/submodules/repo_b/B_A"], - // ), + // Changeset that deletes the submodule metadata file + ExpectedChangeset::new_by_file_change( + DELETE_METADATA_FILE_MSG, + // The submodule files are treated as regular file changes + vec![ + "repo_a/submodules/repo_b/B_A", + "repo_a/submodules/repo_b/B_B", + ], + // Only submodule metadata file is deleted + vec!["repo_a/submodules/.x-repo-submodule-repo_b"], + ), + // Changeset that modifies files in the submodule path, which is + // now a static copy of the submodule + ExpectedChangeset::new_by_file_change( + CHANGE_SUBMODULE_PATH_MSG, + // The submodule files are treated as regular file changes + vec![ + "repo_a/submodules/repo_b/B_B", + "repo_a/submodules/repo_b/B_C", + ], + // Only submodule metadata file is deleted + vec!["repo_a/submodules/repo_b/B_A"], + ), ], )?; - // assert_working_copy_matches_expected( - // &ctx, - // &large_repo, - // large_repo_cs_id, - // vec![ - // "large_repo_root", - // "repo_a/A_A", - // "repo_a/A_B", - // "repo_a/A_C", - // // Files from the submodule are now regular files in the small repo - // "repo_a/submodules/repo_b/B_B", - // "repo_a/submodules/repo_b/B_C", - // ], - // ) - // .await?; + assert_working_copy_matches_expected( + &ctx, + &large_repo, + large_repo_cs_id, + vec![ + "large_repo_root", + "repo_a/A_A", + "repo_a/A_B", + "repo_a/A_C", + // Files from the submodule are now regular files in the small repo + "repo_a/submodules/repo_b/B_B", + "repo_a/submodules/repo_b/B_C", + ], + ) + .await?; // Check mappings of both commits check_mapping( ctx.clone(), &commit_syncer, del_md_file_cs_id, - None, - // Some( - // ChangesetId::from_str( - // "6176e1966404d7da39e62f814ddc49181cb7a31f63a40504f76feada0a47bcf4", - // ) - // .unwrap(), - // ), + Some( + ChangesetId::from_str( + "6176e1966404d7da39e62f814ddc49181cb7a31f63a40504f76feada0a47bcf4", + ) + .unwrap(), + ), ) .await; check_mapping( ctx.clone(), &commit_syncer, chg_sm_path_cs_id, - None, - // Some( - // ChangesetId::from_str( - // "d988d913f6b9f1e91d1fdc41d317cf2d6bed335774c2d5415e33eaa46d086442", - // ) - // .unwrap(), - // ), + Some( + ChangesetId::from_str( + "d988d913f6b9f1e91d1fdc41d317cf2d6bed335774c2d5415e33eaa46d086442", + ) + .unwrap(), + ), ) .await; @@ -743,28 +741,28 @@ async fn test_deleting_recursive_submodule_but_keeping_directory(fb: FacebookIni let large_repo_cs_id = sync_to_master(ctx.clone(), &commit_syncer, del_md_file_cs_id) .await .context("Failed to sync del_md_file_cs_id") - .and_then(|res| res.ok_or(anyhow!("No commit was synced"))); + .and_then(|res| res.ok_or(anyhow!("No commit was synced")))?; println!("large_repo_cs_id: {0:#?}", large_repo_cs_id); - // assert_working_copy_matches_expected( - // &ctx, - // &large_repo, - // large_repo_cs_id, - // vec![ - // "large_repo_root", - // "repo_a/A_A", - // "repo_a/A_B", - // "repo_a/A_C", - // // Files from the submodule are now regular files in the small repo - // "repo_a/submodules/.x-repo-submodule-repo_b", - // "repo_a/submodules/repo_b/B_A", - // "repo_a/submodules/repo_b/B_B", - // "repo_a/submodules/repo_b/submodules/repo_c/C_A", - // "repo_a/submodules/repo_b/submodules/repo_c/C_B", - // ], - // ) - // .await?; + assert_working_copy_matches_expected( + &ctx, + &large_repo, + large_repo_cs_id, + vec![ + "large_repo_root", + "repo_a/A_A", + "repo_a/A_B", + "repo_a/A_C", + // Files from the submodule are now regular files in the small repo + "repo_a/submodules/.x-repo-submodule-repo_b", + "repo_a/submodules/repo_b/B_A", + "repo_a/submodules/repo_b/B_B", + "repo_a/submodules/repo_b/submodules/repo_c/C_A", + "repo_a/submodules/repo_b/submodules/repo_c/C_B", + ], + ) + .await?; const CHANGE_SUBMODULE_PATH_MSG: &str = "Change static copy of repo_c"; @@ -810,90 +808,88 @@ async fn test_deleting_recursive_submodule_but_keeping_directory(fb: FacebookIni let large_repo_cs_id = sync_to_master(ctx.clone(), &commit_syncer, chg_sm_path_cs_id) .await .context("Failed to sync chg_sm_path_cs_id") - .and_then(|res| res.ok_or(anyhow!("No commit was synced"))); + .and_then(|res| res.ok_or(anyhow!("No commit was synced")))?; println!("large_repo_cs_id: {0:#?}", large_repo_cs_id); let large_repo_changesets = get_all_changeset_data_from_repo(&ctx, &large_repo).await?; derive_all_data_types_for_repo(&ctx, &large_repo, &large_repo_changesets).await?; - // compare_expected_changesets( - // large_repo_changesets.last_chunk::<2>().unwrap(), - // &[ - // // Changeset that deletes the submodule metadata file - // ExpectedChangeset::new_by_file_change( - // DELETE_METADATA_FILE_MSG, - // // The submodule files are treated as regular file changes - // vec![ - // // repo_b submodule metadata file is updated - // "repo_a/submodules/.x-repo-submodule-repo_b", - // "repo_a/submodules/repo_b/submodules/repo_c/C_A", - // "repo_a/submodules/repo_b/submodules/repo_c/C_B", - // ], - // // Only submodule metadata file is deleted - // vec!["repo_a/submodules/repo_b/submodules/.x-repo-submodule-repo_c"], - // ), - // // Changeset that modifies files in the submodule path, which is - // // now a static copy of the submodule - // ExpectedChangeset::new_by_file_change( - // CHANGE_SUBMODULE_PATH_MSG, - // // The submodule files are treated as regular file changes - // vec![ - // // repo_b submodule metadata file is updated - // "repo_a/submodules/.x-repo-submodule-repo_b", - // "repo_a/submodules/repo_b/submodules/repo_c/C_B", - // "repo_a/submodules/repo_b/submodules/repo_c/C_C", - // ], - // // Only submodule metadata file is deleted - // vec!["repo_a/submodules/repo_b/submodules/repo_c/C_A"], - // ), - // ], - // )?; + compare_expected_changesets( + large_repo_changesets.last_chunk::<2>().unwrap(), + &[ + // Changeset that deletes the submodule metadata file + ExpectedChangeset::new_by_file_change( + DELETE_METADATA_FILE_MSG, + // The submodule files are treated as regular file changes + vec![ + // repo_b submodule metadata file is updated + "repo_a/submodules/.x-repo-submodule-repo_b", + "repo_a/submodules/repo_b/submodules/repo_c/C_A", + "repo_a/submodules/repo_b/submodules/repo_c/C_B", + ], + // Only submodule metadata file is deleted + vec!["repo_a/submodules/repo_b/submodules/.x-repo-submodule-repo_c"], + ), + // Changeset that modifies files in the submodule path, which is + // now a static copy of the submodule + ExpectedChangeset::new_by_file_change( + CHANGE_SUBMODULE_PATH_MSG, + // The submodule files are treated as regular file changes + vec![ + // repo_b submodule metadata file is updated + "repo_a/submodules/.x-repo-submodule-repo_b", + "repo_a/submodules/repo_b/submodules/repo_c/C_B", + "repo_a/submodules/repo_b/submodules/repo_c/C_C", + ], + // Only submodule metadata file is deleted + vec!["repo_a/submodules/repo_b/submodules/repo_c/C_A"], + ), + ], + )?; - // assert_working_copy_matches_expected( - // &ctx, - // &large_repo, - // large_repo_cs_id, - // vec![ - // "large_repo_root", - // "repo_a/A_A", - // "repo_a/A_B", - // "repo_a/A_C", - // // Files from the submodule are now regular files in the small repo - // "repo_a/submodules/.x-repo-submodule-repo_b", - // "repo_a/submodules/repo_b/B_A", - // "repo_a/submodules/repo_b/B_B", - // "repo_a/submodules/repo_b/submodules/repo_c/C_B", - // "repo_a/submodules/repo_b/submodules/repo_c/C_C", - // ], - // ) - // .await?; + assert_working_copy_matches_expected( + &ctx, + &large_repo, + large_repo_cs_id, + vec![ + "large_repo_root", + "repo_a/A_A", + "repo_a/A_B", + "repo_a/A_C", + // Files from the submodule are now regular files in the small repo + "repo_a/submodules/.x-repo-submodule-repo_b", + "repo_a/submodules/repo_b/B_A", + "repo_a/submodules/repo_b/B_B", + "repo_a/submodules/repo_b/submodules/repo_c/C_B", + "repo_a/submodules/repo_b/submodules/repo_c/C_C", + ], + ) + .await?; // Check mappings of both commits check_mapping( ctx.clone(), &commit_syncer, del_md_file_cs_id, - None, - // Some( - // ChangesetId::from_str( - // "727dc8e42f7886bfb3bd919d58724cff6c4b7d5ea42410184b4c0027e53d8c54", - // ) - // .unwrap(), - // ), + Some( + ChangesetId::from_str( + "727dc8e42f7886bfb3bd919d58724cff6c4b7d5ea42410184b4c0027e53d8c54", + ) + .unwrap(), + ), ) .await; check_mapping( ctx.clone(), &commit_syncer, chg_sm_path_cs_id, - None, - // Some( - // ChangesetId::from_str( - // "3b28131208374b55997843bdcacef567aa8b1bb09212d2d3168c30ef056dcd60", - // ) - // .unwrap(), - // ), + Some( + ChangesetId::from_str( + "3b28131208374b55997843bdcacef567aa8b1bb09212d2d3168c30ef056dcd60", + ) + .unwrap(), + ), ) .await;