facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-04 13:57:13 +03:00
Code Issues Projects Releases Wiki Activity

Allow deletion of submodule metadata file in validation

Browse Source 
Summary: Fixes the issue described in D56633699.

Reviewed By: mitrandir77

Differential Revision: D56690832

fbshipit-source-id: ed61affed8c4d3d6c96ff64071b67c2c793cb8b3
This commit is contained in:
Gustavo Galvao Avena 2024-04-29 23:54:11 -07:00 committed by Facebook GitHub Bot
parent 0dd4a55525
commit 7467a4fc2e
4 changed files with 203 additions and 173 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/utils.rs
View File

@ -186,6 +186,21 @@ pub(crate) async fn get_submodule_file_content_id(
cs_id: ChangesetId,
path: &NonRootMPath,
) -> Result<Option<ContentId>> {
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<R>(
ctx: &CoreContext,
repo: &R,
cs_id: ChangesetId,
path: &NonRootMPath,
expected_file_type: FileType,
) -> Result<Option<ContentId>>
where
R: RepoDerivedDataRef + RepoBlobstoreArc,
{
let fsnode_id = repo
.repo_derived_data()
.derive::<RootFsnodeId>(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),

41
eden/mononoke/commit_rewriting/cross_repo_sync/src/git_submodules/validation.rs
View File

@ -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::<Vec<_>>()
.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

16
eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/backsync.rs
View File

@ -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(())
}

302
eden/mononoke/commit_rewriting/cross_repo_sync/test/git_submodules/forward_sync.rs
View File

@ -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;