From 850a9d428289407b2ecd0aac6a6d4eeb8edb1858 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 12 Aug 2019 13:57:16 -0700 Subject: [PATCH 1/2] Check in package/publish if a git submodule is dirty. --- src/cargo/ops/cargo_package.rs | 36 +++++++++- tests/testsuite/git.rs | 117 +++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index f24332db887..16667fcdf0b 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -267,6 +267,24 @@ fn check_repo_state( allow_dirty: bool, ) -> CargoResult> { let workdir = repo.workdir().unwrap(); + + let mut sub_repos = Vec::new(); + open_submodules(repo, &mut sub_repos)?; + // Sort so that longest paths are first, to check nested submodules first. + sub_repos.sort_unstable_by(|a, b| b.0.as_os_str().len().cmp(&a.0.as_os_str().len())); + let submodule_dirty = |path: &Path| -> bool { + sub_repos + .iter() + .filter(|(sub_path, _sub_repo)| path.starts_with(sub_path)) + .any(|(sub_path, sub_repo)| { + let relative = path.strip_prefix(sub_path).unwrap(); + sub_repo + .status_file(relative) + .map(|status| status != git2::Status::CURRENT) + .unwrap_or(false) + }) + }; + let dirty = src_files .iter() .filter(|file| { @@ -274,7 +292,7 @@ fn check_repo_state( if let Ok(status) = repo.status_file(relative) { status != git2::Status::CURRENT } else { - false + submodule_dirty(file) } }) .map(|path| { @@ -300,6 +318,22 @@ fn check_repo_state( Ok(None) } } + + /// Helper to recursively open all submodules. + fn open_submodules( + repo: &git2::Repository, + sub_repos: &mut Vec<(PathBuf, git2::Repository)>, + ) -> CargoResult<()> { + for submodule in repo.submodules()? { + // Ignore submodules that don't open, they are probably not initialized. + // If its files are required, then the verification step should fail. + if let Ok(sub_repo) = submodule.open() { + open_submodules(&sub_repo, sub_repos)?; + sub_repos.push((sub_repo.workdir().unwrap().to_owned(), sub_repo)); + } + } + Ok(()) + } } // Checks for and `bail!` if a source file matches `ROOT/VCS_INFO_FILE`, since diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index db2f4687a4b..1bd2a9d23ad 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2691,3 +2691,120 @@ fn git_fetch_cli_env_clean() { .env("GIT_DIR", git_proj.root().join(".git")) .run(); } + +#[cargo_test] +fn dirty_submodule() { + // `cargo package` warns for dirty file in submodule. + let git_project = git::new("foo", |project| { + project + .file("Cargo.toml", &basic_manifest("foo", "0.5.0")) + // This is necessary because `git::add` is too eager. + .file(".gitignore", "/target") + }) + .unwrap(); + let git_project2 = git::new("src", |project| { + project.no_manifest().file("lib.rs", "pub fn f() {}") + }) + .unwrap(); + + let repo = git2::Repository::open(&git_project.root()).unwrap(); + let url = path2url(git_project2.root()).to_string(); + git::add_submodule(&repo, &url, Path::new("src")); + + // Submodule added, but not committed. + git_project + .cargo("package --no-verify") + .with_status(101) + .with_stderr( + "\ +[WARNING] manifest has no [..] +See [..] +[ERROR] 1 files in the working directory contain changes that were not yet committed into git: + +.gitmodules + +to proceed despite [..] +", + ) + .run(); + + git::commit(&repo); + git_project.cargo("package --no-verify").run(); + + // Modify file, check for warning. + git_project.change_file("src/lib.rs", ""); + git_project + .cargo("package --no-verify") + .with_status(101) + .with_stderr( + "\ +[WARNING] manifest has no [..] +See [..] +[ERROR] 1 files in the working directory contain changes that were not yet committed into git: + +src/lib.rs + +to proceed despite [..] +", + ) + .run(); + // Commit the change. + let sub_repo = git2::Repository::open(git_project.root().join("src")).unwrap(); + git::add(&sub_repo); + git::commit(&sub_repo); + git::add(&repo); + git::commit(&repo); + git_project.cargo("package --no-verify").run(); + + // Try with a nested submodule. + let git_project3 = git::new("bar", |project| project.no_manifest().file("mod.rs", "")).unwrap(); + let url = path2url(git_project3.root()).to_string(); + git::add_submodule(&sub_repo, &url, Path::new("bar")); + git_project + .cargo("package --no-verify") + .with_status(101) + .with_stderr( + "\ +[WARNING] manifest has no [..] +See [..] +[ERROR] 1 files in the working directory contain changes that were not yet committed into git: + +src/.gitmodules + +to proceed despite [..] +", + ) + .run(); + + // Commit the submodule addition. + git::commit(&sub_repo); + git::add(&repo); + git::commit(&repo); + git_project.cargo("package --no-verify").run(); + // Modify within nested submodule. + git_project.change_file("src/bar/mod.rs", "//test"); + git_project + .cargo("package --no-verify") + .with_status(101) + .with_stderr( + "\ +[WARNING] manifest has no [..] +See [..] +[ERROR] 1 files in the working directory contain changes that were not yet committed into git: + +src/bar/mod.rs + +to proceed despite [..] +", + ) + .run(); + // And commit the change. + let sub_sub_repo = git2::Repository::open(git_project.root().join("src/bar")).unwrap(); + git::add(&sub_sub_repo); + git::commit(&sub_sub_repo); + git::add(&sub_repo); + git::commit(&sub_repo); + git::add(&repo); + git::commit(&repo); + git_project.cargo("package --no-verify").run(); +} From 22adaf99b3508e1a1cc9742ec8e0eed616ca5b70 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 13 Aug 2019 16:35:04 -0700 Subject: [PATCH 2/2] Update test to handle user.* git config missing in submodule. --- tests/testsuite/git.rs | 11 ++++------- tests/testsuite/support/git.rs | 8 ++++++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 1bd2a9d23ad..cdc2dd4ef0a 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2695,19 +2695,16 @@ fn git_fetch_cli_env_clean() { #[cargo_test] fn dirty_submodule() { // `cargo package` warns for dirty file in submodule. - let git_project = git::new("foo", |project| { + let (git_project, repo) = git::new_repo("foo", |project| { project .file("Cargo.toml", &basic_manifest("foo", "0.5.0")) // This is necessary because `git::add` is too eager. .file(".gitignore", "/target") - }) - .unwrap(); + }); let git_project2 = git::new("src", |project| { project.no_manifest().file("lib.rs", "pub fn f() {}") - }) - .unwrap(); + }); - let repo = git2::Repository::open(&git_project.root()).unwrap(); let url = path2url(git_project2.root()).to_string(); git::add_submodule(&repo, &url, Path::new("src")); @@ -2757,7 +2754,7 @@ to proceed despite [..] git_project.cargo("package --no-verify").run(); // Try with a nested submodule. - let git_project3 = git::new("bar", |project| project.no_manifest().file("mod.rs", "")).unwrap(); + let git_project3 = git::new("bar", |project| project.no_manifest().file("mod.rs", "")); let url = path2url(git_project3.root()).to_string(); git::add_submodule(&sub_repo, &url, Path::new("bar")); git_project diff --git a/tests/testsuite/support/git.rs b/tests/testsuite/support/git.rs index b9b1c515b0d..1213594c2cd 100644 --- a/tests/testsuite/support/git.rs +++ b/tests/testsuite/support/git.rs @@ -129,11 +129,14 @@ impl Repository { /// Initialize a new repository at the given path. pub fn init(path: &Path) -> git2::Repository { let repo = t!(git2::Repository::init(path)); + default_repo_cfg(&repo); + repo +} + +fn default_repo_cfg(repo: &git2::Repository) { let mut cfg = t!(repo.config()); t!(cfg.set_str("user.email", "foo@bar.com")); t!(cfg.set_str("user.name", "Foo Bar")); - drop(cfg); - repo } /// Create a new git repository with a project. @@ -193,6 +196,7 @@ pub fn add_submodule<'a>( let path = path.to_str().unwrap().replace(r"\", "/"); let mut s = t!(repo.submodule(url, Path::new(&path), false)); let subrepo = t!(s.open()); + default_repo_cfg(&subrepo); t!(subrepo.remote_add_fetch("origin", "refs/heads/*:refs/heads/*")); let mut origin = t!(subrepo.find_remote("origin")); t!(origin.fetch(&[], None, None));