Ignore non-regular files #1629
Ignore non-regular files #1629
Conversation
(very much unfinished, I will probably finish it later this week). My "mkfifo fifo && cargo publish" hangs because cargo tries to tar the fifo as well. This is surprising because neither "git status" nor "git check-ignore -v fifo" list the untracked fifo. Fifo type files are completely ignored by Git since commit 8695c8b (Add "show-files" command to show the list of managed (or non-managed) files., 2005-04-11) which states the reason: > * [...] We currently ignore anything but > * directories and regular files. That's because git doesn't > * handle them at all yet. Maybe that will change some day. It didn't change yet[*]. Try to match Git to minimize surprise. Combined with some extra error handling, this enables some cleanup. [*] But the logic from read_directory() has moved into treat_path().
There was a problem hiding this comment.
Thanks a lot for tackling this!
I am surprised these seemingly sweeping changes are needed when trying to ignored 'special' files, but would always be open to improvements. And changing Option<bool> to bool certainly counts as such.
However, I need this PR to change in structure so that…
- …there is one commit per crate when dealing with plumbing crates.
- if there is a breaking change, that must be indicated as conventional commit message. That's critical to the release process.
- …there is one commit over all changes necessary to adapt to breaking changes (i.e. make it compile after breaking plumbing changes).
I already left a couple of comments, maybe it's too early though and in that case, please ignore.
It's really important to reorganise the commits before continuing though.
Thank you
| .map_or(false, |platform| platform.matching_attributes(out)) | ||
| }, | ||
| ) | ||
| .pattern_matching_relative_path(path, true, &mut |relative_path, case, is_dir, out| { |
There was a problem hiding this comment.
I don't get why this defaults to true now, and would hope there is a test to catch it. If not, definitely a reason to add a couple, probably including a baseline to validate Git works similarly.
| match Kind::try_from(m.file_type()) { | ||
| Ok(kind) => kind, | ||
| Err(_err) => { | ||
| continue; |
There was a problem hiding this comment.
I am surprised these seemingly sweeping changes are needed when trying to ignored 'special' files
Yeah they're not, the only change that's needed is this "continue", which should probably be its own commit.
But I believe other places can be simplified and made more consistent. I may be wrong, I just need to find time to finish it
There was a problem hiding this comment.
One will definitely need tests for that as well, and in the crates you are working in, they are basically done in a test-driven fashion exclusively as the code is way to complex to understand exactly what's going to happen.
But with the re-organization of commits I think all this will come quite naturally.
|
Sorry I got busy with other stuff, I'm not sure when I'll get around to this. |
|
No worries- do what you can, and I will pick it up asap as well. |
|
Thanks for getting this started! Now I have to close this PR in favor of #1727 which implements it in a more minimal fashion and with tests-first. That way we can be far more certain that nothing else breaks. A new release with the fix should be out on the 22nd of December, and I will assure Cargo also gets the update then. |
The main benefit is that it won't facilitate hangs due to attempts to read from untrackable directory entries, like names pipes or sockets. Related to GitoxideLabs/gitoxide#1629
The main benefit is that it won't facilitate hangs due to attempts to read from untrackable directory entries, like names pipes or sockets. Related to GitoxideLabs/gitoxide#1629
The main benefit is that it won't facilitate hangs due to attempts to read from untrackable directory entries, like names pipes or sockets. Related to GitoxideLabs/gitoxide#1629 ### Tasks * [x] upgrade * [x] incorporate updated `gix` [once everything is fixed](GitoxideLabs/gitoxide#1740). * [x] assure tests work ### Postponed It turns out that the new `gix` version doesn't magically fix the FIFO issue, so the following test still fails. It's not super-trivial to fix apparently (I tried), so let's do it in a separate PR. Here is the patch I have so far in case anyone is interested to fix it earlier or wants to share insights :). ```patch commit dfef545eae215f0b9da9f3d4424b52cba7edaec3 Author: Sebastian Thiel <[email protected]> Date: Sun Dec 22 19:05:40 2024 +0100 fix: assure possibly blocking non-files (like FIFOs) won't be picked up for publishing. This would otherwise cause the publish to hang. diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 776590697..c78463a32 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -626,8 +626,11 @@ fn list_files_gix( .filter(|res| { // Don't include Cargo.lock if it is untracked. Packaging will // generate a new one as needed. + // Also don't include untrackable directory entries, like FIFOs. res.as_ref().map_or(true, |item| { - !(item.entry.status == Status::Untracked && item.entry.rela_path == "Cargo.lock") + item.entry.disk_kind != Some(gix::dir::entry::Kind::Untrackable) + && !(item.entry.status == Status::Untracked + && item.entry.rela_path == "Cargo.lock") }) }) .map(|res| res.map(|item| (item.entry.rela_path, item.entry.disk_kind))) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 1740de4ac..1c6b3db89 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -6873,3 +6873,29 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for "#]]) .run(); } + +#[cargo_test] +#[cfg(unix)] +fn simple_with_fifo() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2015" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + std::process::Command::new("mkfifo") + .current_dir(p.root()) + .arg(p.root().join("blocks-when-read")) + .status() + .expect("a FIFO can be created"); + + // If this hangs, Cargo tried to package a FIFO and is reading it forever. + p.cargo("package").run(); +} ```
The main benefit is that it won't facilitate hangs due to attempts to read from untrackable directory entries, like names pipes or sockets. Related to GitoxideLabs/gitoxide#1629
…up for publishing. (#14977) Follow-up of #14975, related to GitoxideLabs/gitoxide#1629. This would otherwise cause the publish to hang if it tries to read from a fifo.
(very much unfinished, I will probably finish it later this week).
My "mkfifo fifo && cargo publish" hangs because cargo tries to tar
the fifo as well. This is surprising because neither "git status"
nor "git check-ignore -v fifo" list the untracked fifo.
Fifo type files are completely ignored by Git since commit 8695c8b
(Add "show-files" command to show the list of managed (or non-managed)
files., 2005-04-11) which states the reason:
It didn't change yet[*]. Try to match Git to minimize surprise.
Combined with some extra error handling, this enables some cleanup.
[*] But the logic from read_directory() has moved into treat_path().