Skip to content

Commit

Permalink
Auto merge of #7245 - ehuss:warn-dirty-submodule, r=alexcrichton
Browse files Browse the repository at this point in the history
Check in package/publish if a git submodule is dirty.

This extends the "dirty" check during `cargo publish` to check files within a submodule.

It is a little crude, since it unconditionally tries to open every submodule even if it is not relevant to the current package. The performance doesn't seem too bad (~2s for rust-lang/rust with 16 submodules).

It's also a little lax, by ignoring uninitialized submodules. Presumably the verification build will fail if the submodule is not initialized. It could be more aggressive, by requiring all submodules to be initialized?

Closes #3622
  • Loading branch information
bors committed Aug 13, 2019
2 parents d19f614 + 22adaf9 commit a6841b8
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 3 deletions.
36 changes: 35 additions & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,32 @@ fn check_repo_state(
allow_dirty: bool,
) -> CargoResult<Option<String>> {
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| {
let relative = file.strip_prefix(workdir).unwrap();
if let Ok(status) = repo.status_file(relative) {
status != git2::Status::CURRENT
} else {
false
submodule_dirty(file)
}
})
.map(|path| {
Expand All @@ -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
Expand Down
114 changes: 114 additions & 0 deletions tests/testsuite/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2691,3 +2691,117 @@ 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, 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")
});
let git_project2 = git::new("src", |project| {
project.no_manifest().file("lib.rs", "pub fn f() {}")
});

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", ""));
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();
}
8 changes: 6 additions & 2 deletions tests/testsuite/support/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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));
Expand Down

0 comments on commit a6841b8

Please sign in to comment.