From 059fe1608590afba6edebf7e13bb927817e98ed3 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 31 Dec 2024 11:54:12 -0500 Subject: [PATCH] fix(package): warn if symlinks checked out as plain text files `cargo package` will warn users when git `core.symlinks` is `false` and some symlinks were checked out as plain files during packaging. Git config [`core.symlinks`] defaults to true when unset. In git-for-windows (and git as well), the config should be set to false explicitly when the repo was created, if symlink support wasn't detected [^1]. We assume the config was always set at creation time and never changed. So, if it is true, we don't bother users with any warning. [^1]: [`core.symlinks`]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks --- src/cargo/ops/cargo_package/vcs.rs | 48 +++++++++++++++++++++++++ src/cargo/sources/path.rs | 56 ++++++++++++++++++++++++++---- tests/testsuite/package.rs | 4 +++ 3 files changed, 101 insertions(+), 7 deletions(-) diff --git a/src/cargo/ops/cargo_package/vcs.rs b/src/cargo/ops/cargo_package/vcs.rs index ce81235fddc..3c447f32b72 100644 --- a/src/cargo/ops/cargo_package/vcs.rs +++ b/src/cargo/ops/cargo_package/vcs.rs @@ -92,6 +92,8 @@ pub fn check_repo_state( return Ok(None); } + warn_symlink_checked_out_as_plain_text_file(gctx, src_files, &repo)?; + debug!( "found (git) Cargo.toml at `{}` in workdir `{}`", path.display(), @@ -111,6 +113,52 @@ pub fn check_repo_state( return Ok(Some(VcsInfo { git, path_in_vcs })); } +/// Warns if any symlinks were checked out as plain text files. +/// +/// Git config [`core.symlinks`] defaults to true when unset. +/// In git-for-windows (and git as well), +/// the config should be set to false explicitly when the repo was created, +/// if symlink support wasn't detected [^1]. +/// +/// We assume the config was always set at creation time and never changed. +/// So, if it is true, we don't bother users with any warning. +/// +/// [^1]: +/// +/// [`core.symlinks`]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks +fn warn_symlink_checked_out_as_plain_text_file( + gctx: &GlobalContext, + src_files: &[PathEntry], + repo: &git2::Repository, +) -> CargoResult<()> { + if repo + .config() + .and_then(|c| c.get_bool("core.symlinks")) + .unwrap_or(true) + { + return Ok(()); + } + + if src_files.iter().any(|f| f.maybe_plain_text_symlink()) { + let mut shell = gctx.shell(); + shell.warn(format_args!( + "found symbolic links that may be checked out as regular files for git repo at `{}`\n\ + This might cause the `.crate` file to include incorrect or incomplete files", + repo.workdir().unwrap().display(), + ))?; + let extra_note = if cfg!(windows) { + "\nAnd on Windows, enable the Developer Mode to support symlinks" + } else { + "" + }; + shell.note(format_args!( + "to avoid this, set the Git config `core.symlinks` to `true`{extra_note}", + ))?; + } + + Ok(()) +} + /// The real git status check starts from here. fn git( pkg: &Package, diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 5ced38da6b5..8bafcf18099 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -407,7 +407,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> { /// Type that abstracts over [`gix::dir::entry::Kind`] and [`fs::FileType`]. #[derive(Debug, Clone, Copy)] enum FileType { - File, + File { maybe_symlink: bool }, Dir, Symlink, Other, @@ -416,7 +416,9 @@ enum FileType { impl From for FileType { fn from(value: fs::FileType) -> Self { if value.is_file() { - FileType::File + FileType::File { + maybe_symlink: false, + } } else if value.is_dir() { FileType::Dir } else if value.is_symlink() { @@ -432,7 +434,9 @@ impl From for FileType { use gix::dir::entry::Kind; match value { Kind::Untrackable => FileType::Other, - Kind::File => FileType::File, + Kind::File => FileType::File { + maybe_symlink: false, + }, Kind::Symlink => FileType::Symlink, Kind::Directory | Kind::Repository => FileType::Dir, } @@ -454,7 +458,7 @@ impl PathEntry { /// Similar to [`std::path::Path::is_file`] /// but doesn't follow the symbolic link nor make any system call pub fn is_file(&self) -> bool { - matches!(self.ty, FileType::File) + matches!(self.ty, FileType::File { .. }) } /// Similar to [`std::path::Path::is_dir`] @@ -468,6 +472,19 @@ impl PathEntry { pub fn is_symlink(&self) -> bool { matches!(self.ty, FileType::Symlink) } + + /// Whether this path might be a plain text symlink. + /// + /// Git may check out symlinks as plain text files that contain the link texts, + /// when either `core.symlinks` is `false`, or on Windows. + pub fn maybe_plain_text_symlink(&self) -> bool { + matches!( + self.ty, + FileType::File { + maybe_symlink: true + } + ) + } } impl std::ops::Deref for PathEntry { @@ -713,7 +730,24 @@ fn list_files_gix( && item.entry.rela_path == "Cargo.lock") }) }) - .map(|res| res.map(|item| (item.entry.rela_path, item.entry.disk_kind))) + .map(|res| { + res.map(|item| { + // Assumption: if a file tracked as a symlink in Git index, and + // the actual file type on disk is file, then it might be a + // plain text file symlink. + // There are exceptions like the file has changed from a symlink + // to a real text file, but hasn't been committed to Git index. + // Exceptions may be rare so we're okay with this now. + let maybe_plain_text_symlink = item.entry.index_kind + == Some(gix::dir::entry::Kind::Symlink) + && item.entry.disk_kind == Some(gix::dir::entry::Kind::File); + ( + item.entry.rela_path, + item.entry.disk_kind, + maybe_plain_text_symlink, + ) + }) + }) .chain( // Append entries that might be tracked in `/target/`. index @@ -731,12 +765,13 @@ fn list_files_gix( // This traversal is not part of a `status()`, and tracking things in `target/` // is rare. None, + false, ) }) .map(Ok), ) { - let (rela_path, kind) = item?; + let (rela_path, kind, maybe_plain_text_symlink) = item?; let file_path = root.join(gix::path::from_bstr(rela_path)); if file_path.file_name().and_then(|name| name.to_str()) == Some("Cargo.toml") { // Keep track of all sub-packages found and also strip out all @@ -781,9 +816,16 @@ fn list_files_gix( } else if (filter)(&file_path, is_dir) { assert!(!is_dir); trace!(" found {}", file_path.display()); + let ty = match kind.map(Into::into) { + Some(FileType::File { .. }) => FileType::File { + maybe_symlink: maybe_plain_text_symlink, + }, + Some(ty) => ty, + None => FileType::Other, + }; files.push(PathEntry { path: file_path, - ty: kind.map(Into::into).unwrap_or(FileType::Other), + ty, }); } } diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 093cf89eb27..9c6d0493f83 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -7077,6 +7077,10 @@ fn git_core_symlinks_false() { p.cargo("package --allow-dirty") .with_stderr_data(str![[r#" +[WARNING] found symbolic links that may be checked out as regular files for git repo at `[ROOT]/foo/` +This might cause the `.crate` file to include incorrect or incomplete files +[NOTE] to avoid this, set the Git config `core.symlinks` to `true` +... [PACKAGING] bar v0.0.0 ([ROOT]/foo) [PACKAGED] 7 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) [VERIFYING] bar v0.0.0 ([ROOT]/foo)