From 0d517716b81e203a131552c5163894a4fbf2863e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 19 Dec 2024 08:45:12 +0100 Subject: [PATCH 1/3] fix!: Add `entry::Kind::NonFile`. Previously, these were misclassified as `File`, which can lead to blocking applications which get stuck reading pipes. Now the downstream is forced to deal with the possibility that the item at hand isn't a file, to do application-specific things. --- gix-dir/src/entry.rs | 22 ++++++++++++---------- gix-dir/src/walk/classify.rs | 10 ++-------- gix-dir/src/walk/readdir.rs | 2 +- gix-dir/tests/dir/walk.rs | 11 ++++++++--- 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/gix-dir/src/entry.rs b/gix-dir/src/entry.rs index a34885b1b75..51dd41d22e0 100644 --- a/gix-dir/src/entry.rs +++ b/gix-dir/src/entry.rs @@ -1,6 +1,7 @@ use crate::walk::ForDeletionMode; use crate::{Entry, EntryRef}; use std::borrow::Cow; +use std::fs::FileType; /// A way of attaching additional information to an [Entry] . #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)] @@ -25,6 +26,10 @@ pub enum Property { /// The kind of the entry, seated in their kinds available on disk. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)] pub enum Kind { + /// Something that is not a file, like a named pipe or character device. + /// + /// These can only exist in the filesystem. + NonFile, /// The entry is a blob, executable or not. File, /// The entry is a symlink. @@ -147,20 +152,17 @@ impl Entry { } } -impl Kind { - /// Try to represent the file type `t` as `Entry`, or return `None` if it cannot be represented. - /// - /// The latter can happen if it's a `pipe` for instance. - pub fn try_from_file_type(t: std::fs::FileType) -> Option { - Some(if t.is_dir() { +impl From for Kind { + fn from(value: FileType) -> Self { + if value.is_dir() { Kind::Directory - } else if t.is_symlink() { + } else if value.is_symlink() { Kind::Symlink - } else if t.is_file() { + } else if value.is_file() { Kind::File } else { - return None; - }) + Kind::NonFile + } } } diff --git a/gix-dir/src/walk/classify.rs b/gix-dir/src/walk/classify.rs index e289307f792..2fbae8753aa 100644 --- a/gix-dir/src/walk/classify.rs +++ b/gix-dir/src/walk/classify.rs @@ -20,10 +20,7 @@ pub fn root( let mut last_length = None; let mut path_buf = worktree_root.to_owned(); // These initial values kick in if worktree_relative_root.is_empty(); - let file_kind = path_buf - .symlink_metadata() - .ok() - .and_then(|m| entry::Kind::try_from_file_type(m.file_type())); + let file_kind = path_buf.symlink_metadata().ok().map(|m| m.file_type().into()); let mut out = path(&mut path_buf, buf, 0, file_kind, || None, options, ctx)?; let worktree_root_is_repository = out .disk_kind @@ -35,10 +32,7 @@ pub fn root( } path_buf.push(component); buf.extend_from_slice(gix_path::os_str_into_bstr(component.as_os_str()).expect("no illformed UTF8")); - let file_kind = path_buf - .symlink_metadata() - .ok() - .and_then(|m| entry::Kind::try_from_file_type(m.file_type())); + let file_kind = path_buf.symlink_metadata().ok().map(|m| m.file_type().into()); out = path( &mut path_buf, diff --git a/gix-dir/src/walk/readdir.rs b/gix-dir/src/walk/readdir.rs index cde07fa0ea6..ab1e9370344 100644 --- a/gix-dir/src/walk/readdir.rs +++ b/gix-dir/src/walk/readdir.rs @@ -64,7 +64,7 @@ pub(super) fn recursive( current_bstr, if prev_len == 0 { 0 } else { prev_len + 1 }, None, - || entry.file_type().ok().and_then(entry::Kind::try_from_file_type), + || entry.file_type().ok().map(Into::into), opts, ctx, )?; diff --git a/gix-dir/tests/dir/walk.rs b/gix-dir/tests/dir/walk.rs index 36b6bd591f4..10673e34f48 100644 --- a/gix-dir/tests/dir/walk.rs +++ b/gix-dir/tests/dir/walk.rs @@ -66,7 +66,11 @@ fn one_top_level_fifo() { } ); - assert_eq!(entries, &[], "Non-files are simply pruned by default"); + assert_eq!( + entries, + &[entry("top", Untracked, NonFile),], + "Non-files are like normal files, but with a different state" + ); } #[test] @@ -99,10 +103,11 @@ fn fifo_in_traversal() { &[ entry_nokind(".git", Pruned).with_property(DotGit).with_match(Always), entry("dir-with-file/nested-file", Untracked, File), + entry("dir/nested", Untracked, NonFile), entry("file", Untracked, File), + entry("top", Untracked, NonFile), ], - "Non-files are not even pruned, they are ignored entirely.\ - If one day this isn't what we want, we can create an own filetype for them" + "Non-files only differ by their disk-kind" ); } From a7c41008705b3c2777b467c60e5b5881021c5abf Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 19 Dec 2024 14:38:17 +0100 Subject: [PATCH 2/3] adapt to changes in `gix-dir` --- gitoxide-core/src/repository/clean.rs | 18 +++++++++++++++--- .../src/index_as_worktree_with_renames/mod.rs | 16 ++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/gitoxide-core/src/repository/clean.rs b/gitoxide-core/src/repository/clean.rs index a21bbe25a6c..0a05d4c0e7d 100644 --- a/gitoxide-core/src/repository/clean.rs +++ b/gitoxide-core/src/repository/clean.rs @@ -161,11 +161,16 @@ pub(crate) mod function { if entry.disk_kind.is_none() { entry.disk_kind = workdir .join(gix::path::from_bstr(entry.rela_path.as_bstr())) - .metadata() + .symlink_metadata() .ok() - .and_then(|e| gix::dir::entry::Kind::try_from_file_type(e.file_type())); + .map(|e| e.file_type().into()); } - let mut disk_kind = entry.disk_kind.expect("present if not pruned"); + let Some(mut disk_kind) = entry.disk_kind else { + if debug { + writeln!(err, "DBG: ignoring unreadable entry at '{}' ", entry.rela_path).ok(); + } + continue; + }; if !keep { if debug { writeln!(err, "DBG: prune '{}' as -x or -p is missing", entry.rela_path).ok(); @@ -183,6 +188,12 @@ pub(crate) mod function { } match disk_kind { + Kind::NonFile => { + if debug { + writeln!(err, "DBG: skipped non-file at '{}'", entry.rela_path).ok(); + } + continue; + } Kind::File | Kind::Symlink => {} Kind::Directory => { if !directories { @@ -254,6 +265,7 @@ pub(crate) mod function { "WOULD remove" }, suffix = match disk_kind { + Kind::NonFile => unreachable!("always skipped earlier"), Kind::Directory if entry.property == Some(gix::dir::entry::Property::EmptyDirectory) => { " empty" } diff --git a/gix-status/src/index_as_worktree_with_renames/mod.rs b/gix-status/src/index_as_worktree_with_renames/mod.rs index f2202ea0869..ed6f7fcbefc 100644 --- a/gix-status/src/index_as_worktree_with_renames/mod.rs +++ b/gix-status/src/index_as_worktree_with_renames/mod.rs @@ -158,7 +158,7 @@ pub(super) mod function { let tracker = options .rewrites - .map(gix_diff::rewrites::Tracker::>::new) + .map(gix_diff::rewrites::Tracker::>::new) .zip(filter); let rewrite_outcome = match tracker { Some((mut tracker, (mut filter, mut attrs))) => { @@ -168,12 +168,12 @@ pub(super) mod function { let (change, location) = match event { Event::IndexEntry(record) => { let location = Cow::Borrowed(record.relative_path); - (rewrite::ModificationOrDirwalkEntry::Modification(record), location) + (ModificationOrDirwalkEntry::Modification(record), location) } Event::DirEntry(entry, collapsed_directory_status) => { let location = Cow::Owned(entry.rela_path.clone()); ( - rewrite::ModificationOrDirwalkEntry::DirwalkEntry { + ModificationOrDirwalkEntry::DirwalkEntry { id: rewrite::calculate_worktree_id( options.object_hash, worktree, @@ -222,7 +222,7 @@ pub(super) mod function { } } Some(src) => { - let rewrite::ModificationOrDirwalkEntry::DirwalkEntry { + let ModificationOrDirwalkEntry::DirwalkEntry { id, entry, collapsed_directory_status, @@ -466,6 +466,10 @@ pub(super) mod function { ModificationOrDirwalkEntry::Modification(c) => c.entry.mode.to_tree_entry_mode(), ModificationOrDirwalkEntry::DirwalkEntry { entry, .. } => entry.disk_kind.map(|kind| { match kind { + Kind::NonFile => { + // Trees are never tracked for rewrites, so we 'pretend'. + gix_object::tree::EntryKind::Tree + } Kind::File => gix_object::tree::EntryKind::Blob, Kind::Symlink => gix_object::tree::EntryKind::Link, Kind::Repository | Kind::Directory => gix_object::tree::EntryKind::Tree, @@ -500,6 +504,10 @@ pub(super) mod function { }; Ok(match kind { + Kind::NonFile => { + // Go along with unreadable files, they are passed along without rename tracking. + return Ok(object_hash.null()); + } Kind::File => { let platform = attrs .at_entry(rela_path, None, objects) From 3614c21da2357cb58b7e8572ca49b52656780594 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 19 Dec 2024 15:13:41 +0100 Subject: [PATCH 3/3] fix: `status` retrieval now deals with non-file entries by ignoring them when possible. --- gix-status/src/index_as_worktree/types.rs | 7 ++- .../src/index_as_worktree_with_renames/mod.rs | 7 ++- gix-status/tests/Cargo.toml | 4 +- .../fixtures/generated-archives/.gitignore | 1 + gix-status/tests/fixtures/status_nonfile.sh | 19 ++++++++ gix-status/tests/status/index_as_worktree.rs | 17 ++++++++ .../status/index_as_worktree_with_renames.rs | 43 ++++++++++++++++--- gix-status/tests/status/mod.rs | 4 ++ .../tests/{stack/mod.rs => status/stack.rs} | 0 gix-status/tests/worktree.rs | 4 -- 10 files changed, 92 insertions(+), 14 deletions(-) create mode 100755 gix-status/tests/fixtures/status_nonfile.sh rename gix-status/tests/{stack/mod.rs => status/stack.rs} (100%) delete mode 100644 gix-status/tests/worktree.rs diff --git a/gix-status/src/index_as_worktree/types.rs b/gix-status/src/index_as_worktree/types.rs index 729171f4595..446ac8ce250 100644 --- a/gix-status/src/index_as_worktree/types.rs +++ b/gix-status/src/index_as_worktree/types.rs @@ -103,7 +103,12 @@ impl Outcome { pub enum Change { /// This corresponding file does not exist in the worktree anymore. Removed, - /// The type of file changed compared to the worktree, i.e. a symlink s now a file. + /// The type of file changed compared to the worktree, i.e. a symlink is now a file, or a file was replaced with a named pipe. + /// + /// ### Deviation + /// + /// A change to a non-file is marked as `modification` in Git, but that's related to the content which we can't evaluate. + /// Hence, a type-change is considered more appropriate. Type, /// This worktree file was modified in some form, like a permission change or content change or both, /// as compared to this entry. diff --git a/gix-status/src/index_as_worktree_with_renames/mod.rs b/gix-status/src/index_as_worktree_with_renames/mod.rs index ed6f7fcbefc..24a220e5a90 100644 --- a/gix-status/src/index_as_worktree_with_renames/mod.rs +++ b/gix-status/src/index_as_worktree_with_renames/mod.rs @@ -387,8 +387,11 @@ pub(super) mod function { impl gix_dir::walk::Delegate for Delegate<'_, '_, T, U> { fn emit(&mut self, entry: EntryRef<'_>, collapsed_directory_status: Option) -> Action { - let entry = entry.to_owned(); - self.tx.send(Event::DirEntry(entry, collapsed_directory_status)).ok(); + // Status never shows untracked non-files + if entry.disk_kind != Some(gix_dir::entry::Kind::NonFile) { + let entry = entry.to_owned(); + self.tx.send(Event::DirEntry(entry, collapsed_directory_status)).ok(); + } if self.should_interrupt.load(Ordering::Relaxed) { Action::Cancel diff --git a/gix-status/tests/Cargo.toml b/gix-status/tests/Cargo.toml index 167e2ff0d3e..3a1ae17391c 100644 --- a/gix-status/tests/Cargo.toml +++ b/gix-status/tests/Cargo.toml @@ -12,8 +12,8 @@ publish = false rust-version = "1.65" [[test]] -name = "worktree" -path = "worktree.rs" +name = "status" +path = "status/mod.rs" [features] gix-features-parallel = ["gix-features/parallel"] diff --git a/gix-status/tests/fixtures/generated-archives/.gitignore b/gix-status/tests/fixtures/generated-archives/.gitignore index 54c8e16e9b6..4fef5783641 100644 --- a/gix-status/tests/fixtures/generated-archives/.gitignore +++ b/gix-status/tests/fixtures/generated-archives/.gitignore @@ -1,3 +1,4 @@ status_unchanged.tar status_changed.tar symlink_stack.tar +status_nonfile.tar diff --git a/gix-status/tests/fixtures/status_nonfile.sh b/gix-status/tests/fixtures/status_nonfile.sh new file mode 100755 index 00000000000..3f1a8b55de1 --- /dev/null +++ b/gix-status/tests/fixtures/status_nonfile.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash +set -eu -o pipefail + +git init -q untracked +(cd untracked + touch file && git add file && git commit -m "just to get an index for the test-suite" + + mkfifo pipe + git status +) + +git init -q tracked-swapped +(cd tracked-swapped + touch file && git add file && git commit -m "it starts out as trackable file" + + rm file && mkfifo file + git status +) + diff --git a/gix-status/tests/status/index_as_worktree.rs b/gix-status/tests/status/index_as_worktree.rs index eee3c656546..27a3a272c0b 100644 --- a/gix-status/tests/status/index_as_worktree.rs +++ b/gix-status/tests/status/index_as_worktree.rs @@ -37,6 +37,10 @@ fn fixture(name: &str, expected_status: &[Expectation<'_>]) -> Outcome { fixture_filtered(name, &[], expected_status) } +fn nonfile_fixture(name: &str, expected_status: &[Expectation<'_>]) -> Outcome { + fixture_filtered_detailed("status_nonfile", name, &[], expected_status, |_| {}, false) +} + fn fixture_with_index( name: &str, prepare_index: impl FnMut(&mut gix_index::State), @@ -185,6 +189,19 @@ fn status_removed() -> EntryStatus { Change::Removed.into() } +#[test] +#[cfg(unix)] +fn nonfile_untracked_are_not_visible() { + // And generally, untracked aren't visible here. + nonfile_fixture("untracked", &[]); +} + +#[test] +#[cfg(unix)] +fn tracked_changed_to_non_file() { + nonfile_fixture("tracked-swapped", &[(BStr::new(b"file"), 0, Change::Type.into())]); +} + #[test] fn removed() { let out = fixture( diff --git a/gix-status/tests/status/index_as_worktree_with_renames.rs b/gix-status/tests/status/index_as_worktree_with_renames.rs index 89117dfa350..5ed86f41213 100644 --- a/gix-status/tests/status/index_as_worktree_with_renames.rs +++ b/gix-status/tests/status/index_as_worktree_with_renames.rs @@ -1,4 +1,4 @@ -use crate::status::fixture_path; +use crate::fixture_path; use bstr::ByteSlice; use gix_diff::blob::pipeline::WorktreeRoots; use gix_diff::rewrites::CopySource; @@ -83,6 +83,7 @@ fn changed_and_untracked_and_renamed() { track_empty: true, }; let out = fixture_filtered_detailed( + "status_many.sh", "changed-and-untracked-and-renamed", &[], &expectations_with_dirwalk, @@ -100,9 +101,38 @@ fn changed_and_untracked_and_renamed() { ); } +#[test] +#[cfg(unix)] +fn nonfile_untracked_are_not_visible() { + fixture_filtered_detailed( + "status_nonfile.sh", + "untracked", + &[], + &[], + None, + Some(Default::default()), + ); +} + +#[test] +#[cfg(unix)] +fn tracked_changed_to_non_file() { + fixture_filtered_detailed( + "status_nonfile.sh", + "tracked-swapped", + &[], + &[Expectation::Modification { + rela_path: "file", + status: Change::Type.into(), + }], + None, + Some(Default::default()), + ); +} #[test] fn changed_and_untracked() { let out = fixture_filtered_detailed( + "status_many.sh", "changed-and-untracked", &[], &[Expectation::Modification { @@ -144,6 +174,7 @@ fn changed_and_untracked() { }, ]; let out = fixture_filtered_detailed( + "status_many.sh", "changed-and-untracked", &[], &expectations_with_dirwalk, @@ -163,6 +194,7 @@ fn changed_and_untracked() { assert_eq!(out.rewrites, None, "rewrites are still not configured"); let out = fixture_filtered_detailed( + "status_many.sh", "changed-and-untracked", &[], &expectations_with_dirwalk, @@ -179,6 +211,7 @@ fn changed_and_untracked() { } fn fixture_filtered_detailed( + script: &str, subdir: &str, pathspecs: &[&str], expected: &[Expectation<'_>], @@ -193,11 +226,11 @@ fn fixture_filtered_detailed( out } - let worktree = fixture_path("status_many.sh").join(subdir); + let worktree = fixture_path(script).join(subdir); let git_dir = worktree.join(".git"); let index = gix_index::File::at(git_dir.join("index"), gix_hash::Kind::Sha1, false, Default::default()).unwrap(); let search = gix_pathspec::Search::from_specs( - crate::status::index_as_worktree::to_pathspecs(pathspecs), + crate::index_as_worktree::to_pathspecs(pathspecs), None, std::path::Path::new(""), ) @@ -247,7 +280,7 @@ fn fixture_filtered_detailed( object_hash: gix_hash::Kind::Sha1, tracked_file_modifications: gix_status::index_as_worktree::Options { fs: capabilities, - stat: crate::status::index_as_worktree::TEST_OPTIONS, + stat: crate::index_as_worktree::TEST_OPTIONS, ..Default::default() }, dirwalk, @@ -262,7 +295,7 @@ fn fixture_filtered_detailed( &worktree, &mut recorder, FastEq, - crate::status::index_as_worktree::SubmoduleStatusMock { dirty: false }, + crate::index_as_worktree::SubmoduleStatusMock { dirty: false }, objects, &mut gix_features::progress::Discard, context, diff --git a/gix-status/tests/status/mod.rs b/gix-status/tests/status/mod.rs index 4da345b7a8d..43135b103db 100644 --- a/gix-status/tests/status/mod.rs +++ b/gix-status/tests/status/mod.rs @@ -1,6 +1,10 @@ +pub use gix_testtools::Result; + mod index_as_worktree; mod index_as_worktree_with_renames; +mod stack; + pub fn fixture_path(name: &str) -> std::path::PathBuf { let dir = gix_testtools::scripted_fixture_read_only_standalone(std::path::Path::new(name).with_extension("sh")) .expect("script works"); diff --git a/gix-status/tests/stack/mod.rs b/gix-status/tests/status/stack.rs similarity index 100% rename from gix-status/tests/stack/mod.rs rename to gix-status/tests/status/stack.rs diff --git a/gix-status/tests/worktree.rs b/gix-status/tests/worktree.rs deleted file mode 100644 index 8508f38a9c4..00000000000 --- a/gix-status/tests/worktree.rs +++ /dev/null @@ -1,4 +0,0 @@ -pub use gix_testtools::Result; -mod stack; -mod status; -use status::*;