Skip to content

Commit

Permalink
Auto merge of #13696 - arlosi:gitoxide-list-files-default, r=weihanglo
Browse files Browse the repository at this point in the history
Switch to using gitoxide by default for listing files

### What does this PR try to resolve?

Uses gitoxide by for listing the contents of a git repository by default. Fixes #10150

It's possible out-opt of this change with the environment variable `__CARGO_GITOXIDE_DISABLE_LIST_FILES=1`. This opt-out mechanism is temporary and will be removed before the next release.

### How should we test and review this PR?

The newly added test fails with the `git2` implementation.
  • Loading branch information
bors committed Apr 4, 2024
2 parents 6774b85 + 312e2aa commit 81ca704
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 14 deletions.
8 changes: 1 addition & 7 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -908,8 +908,6 @@ fn parse_git(it: impl Iterator<Item = impl AsRef<str>>) -> CargoResult<Option<Gi
pub struct GitoxideFeatures {
/// All fetches are done with `gitoxide`, which includes git dependencies as well as the crates index.
pub fetch: bool,
/// Listing of files suitable for packaging with Git support.
pub list_files: bool,
/// Checkout git dependencies using `gitoxide` (submodules are still handled by git2 ATM, and filters
/// like linefeed conversions are unsupported).
pub checkout: bool,
Expand All @@ -923,7 +921,6 @@ impl GitoxideFeatures {
fn all() -> Self {
GitoxideFeatures {
fetch: true,
list_files: true,
checkout: true,
internal_use_git2: false,
}
Expand All @@ -934,7 +931,6 @@ impl GitoxideFeatures {
fn safe() -> Self {
GitoxideFeatures {
fetch: true,
list_files: true,
checkout: true,
internal_use_git2: false,
}
Expand All @@ -947,7 +943,6 @@ fn parse_gitoxide(
let mut out = GitoxideFeatures::default();
let GitoxideFeatures {
fetch,
list_files,
checkout,
internal_use_git2,
} = &mut out;
Expand All @@ -956,10 +951,9 @@ fn parse_gitoxide(
match e.as_ref() {
"fetch" => *fetch = true,
"checkout" => *checkout = true,
"list-files" => *list_files = true,
"internal-use-git2" => *internal_use_git2 = true,
_ => {
bail!("unstable 'gitoxide' only takes `fetch`, `list-files` and 'checkout' as valid input, for shallow fetches see `-Zgit=shallow-index,shallow-deps`")
bail!("unstable 'gitoxide' only takes `fetch` and 'checkout' as valid input, for shallow fetches see `-Zgit=shallow-index,shallow-deps`")
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -533,8 +533,9 @@ fn check_repo_state(
if let Some(workdir) = repo.workdir() {
debug!("found a git repo at {:?}", workdir);
let path = p.manifest_path();
let path = path.strip_prefix(workdir).unwrap_or(path);
if let Ok(status) = repo.status_file(path) {
let path =
paths::strip_prefix_canonical(path, workdir).unwrap_or_else(|_| path.to_path_buf());
if let Ok(status) = repo.status_file(&path) {
if (status & git2::Status::IGNORED).is_empty() {
debug!(
"found (git) Cargo.toml at {:?} in workdir {:?}",
Expand Down
11 changes: 6 additions & 5 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,14 @@ impl<'gctx> PathSource<'gctx> {
let git_repo = if no_include_option {
if self
.gctx
.cli_unstable()
.gitoxide
.map_or(false, |features| features.list_files)
.get_env("__CARGO_GITOXIDE_DISABLE_LIST_FILES")
.ok()
.as_deref()
== Some("1")
{
self.discover_gix_repo(root)?.map(Git2OrGixRepository::Gix)
} else {
self.discover_git_repo(root)?.map(Git2OrGixRepository::Git2)
} else {
self.discover_gix_repo(root)?.map(Git2OrGixRepository::Gix)
}
} else {
None
Expand Down
33 changes: 33 additions & 0 deletions tests/testsuite/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3510,3 +3510,36 @@ Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and poin
.with_stderr(&expect_msg)
.run();
}

#[cargo_test]
fn symlink_manifest_path() {
// Test `cargo install --manifest-path` pointing through a symlink.
if !symlink_supported() {
return;
}
let p = git::new("foo", |p| {
p.file("Cargo.toml", &basic_manifest("foo", "1.0.0"))
.file("src/main.rs", "fn main() {}")
// Triggers discover_git_and_list_files for detecting changed files.
.file("build.rs", "fn main() {}")
});
#[cfg(unix)]
use std::os::unix::fs::symlink;
#[cfg(windows)]
use std::os::windows::fs::symlink_dir as symlink;

let foo_symlink = paths::root().join("foo-symlink");
t!(symlink(p.root(), &foo_symlink));

cargo_process("package --no-verify --manifest-path")
.arg(foo_symlink.join("Cargo.toml"))
.with_stderr(
"\
warning: manifest has no description, license, license-file, documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
[PACKAGING] foo v1.0.0 ([..]foo-symlink)
[PACKAGED] 6 files[..]
",
)
.run()
}

0 comments on commit 81ca704

Please sign in to comment.