diff --git a/doc_src/interactive.rst b/doc_src/interactive.rst index 22256ec77..56fbe8c53 100644 --- a/doc_src/interactive.rst +++ b/doc_src/interactive.rst @@ -133,7 +133,7 @@ Variable Meaning .. envvar:: fish_color_end process separators like ``;`` and ``&`` .. envvar:: fish_color_error syntax errors .. envvar:: fish_color_param ordinary command parameters -.. envvar:: fish_color_valid_path parameters that are filenames (if the file exists) +.. envvar:: fish_color_valid_path parameters and redirection targets that are filenames (if the file exists) .. envvar:: fish_color_option options starting with "-", up to the first "--" parameter .. envvar:: fish_color_comment comments like '# important' .. envvar:: fish_color_selection selected text in vi visual mode diff --git a/src/highlight/file_tester.rs b/src/highlight/file_tester.rs index 32f986a08..56a40db8f 100644 --- a/src/highlight/file_tester.rs +++ b/src/highlight/file_tester.rs @@ -129,17 +129,17 @@ pub fn test_cd_path(&self, token: &wstr, is_prefix: bool) -> FileTestResult { } } - // Test if a the given string is a valid redirection target, given the mode. - // Note we return bool, because we never underline redirection targets. - pub fn test_redirection_target(&self, target: &wstr, mode: RedirectionMode) -> bool { + // Test if a the given string is a valid redirection target, and if so, whether + // it is a path to an existing file. + pub fn test_redirection_target(&self, target: &wstr, mode: RedirectionMode) -> FileTestResult { // Skip targets exceeding PATH_MAX. See #7837. if target.len() > (PATH_MAX as usize) { - return false; + return Err(IsErr); } let mut target = target.to_owned(); if !expand_one(&mut target, ExpandFlags::FAIL_ON_CMDSUBST, self.ctx, None) { // Could not be expanded. - return false; + return Err(IsErr); } // Ok, we successfully expanded our target. Now verify that it works with this // redirection. We will probably need it as a path (but not in the case of fd @@ -148,29 +148,33 @@ pub fn test_redirection_target(&self, target: &wstr, mode: RedirectionMode) -> b match mode { RedirectionMode::Fd => { if target == "-" { - return true; + return Ok(IsFile(false)); } match fish_wcstoi(&target) { - Ok(fd) => fd >= 0, - Err(_) => false, + Ok(fd) if fd >= 0 => Ok(IsFile(false)), + _ => Err(IsErr), } } RedirectionMode::Input | RedirectionMode::TryInput => { // Input redirections must have a readable non-directory. // Note we color "try_input" files as errors if they are invalid, // even though it's possible to execute these (replaced via /dev/null). - waccess(&target_path, libc::R_OK) == 0 + if waccess(&target_path, libc::R_OK) == 0 && wstat(&target_path).is_ok_and(|md| !md.file_type().is_dir()) + { + Ok(IsFile(true)) + } else { + Err(IsErr) + } } RedirectionMode::Overwrite | RedirectionMode::Append | RedirectionMode::NoClob => { if string_suffixes_string(L!("/"), &target) { // Redirections to things that are directories is definitely not // allowed. - return false; + return Err(IsErr); } // Test whether the file exists, and whether it's writable (possibly after // creating it). access() returns failure if the file does not exist. - // TODO: we do not need to compute file_exists for an 'overwrite' redirection. let file_exists; let file_is_writable; match wstat(&target_path) { @@ -206,7 +210,10 @@ pub fn test_redirection_target(&self, target: &wstr, mode: RedirectionMode) -> b } } // NoClob means that we must not overwrite files that exist. - file_is_writable && !(file_exists && mode == RedirectionMode::NoClob) + if !file_is_writable || (mode == RedirectionMode::NoClob && file_exists) { + return Err(IsErr); + } + Ok(IsFile(file_exists)) } } } @@ -546,57 +553,57 @@ fn test_redirections() { // Normal redirection. let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::Input); - assert!(result); + assert_eq!(result, Ok(IsFile(true))); // Can't redirect from a missing file let result = tester.test_redirection_target(L!("notfile.txt"), RedirectionMode::Input); - assert!(!result); + assert_eq!(result, Err(IsErr)); let result = tester.test_redirection_target(L!("bogus_path/file.txt"), RedirectionMode::Input); - assert!(!result); + assert_eq!(result, Err(IsErr)); // Can't redirect from a directory. let result = tester.test_redirection_target(L!("somedir"), RedirectionMode::Input); - assert!(!result); + assert_eq!(result, Err(IsErr)); // Can't redirect from an unreadable file. #[cfg(not(cygwin))] // Can't mark a file write-only on MSYS, this may work on true Cygwin { fs::set_permissions(&file_path, Permissions::from_mode(0o200)).unwrap(); let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::Input); - assert!(!result); + assert_eq!(result, Err(IsErr)); fs::set_permissions(&file_path, Permissions::from_mode(0o600)).unwrap(); } // try_input syntax highlighting reports an error even though the command will succeed. let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::TryInput); - assert!(result); + assert_eq!(result, Ok(IsFile(true))); let result = tester.test_redirection_target(L!("notfile.txt"), RedirectionMode::TryInput); - assert!(!result); + assert_eq!(result, Err(IsErr)); let result = tester.test_redirection_target(L!("bogus_path/file.txt"), RedirectionMode::TryInput); - assert!(!result); + assert_eq!(result, Err(IsErr)); // Test write redirections. // Overwrite an existing file. let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::Overwrite); - assert!(result); + assert_eq!(result, Ok(IsFile(true))); // Append to an existing file. let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::Append); - assert!(result); + assert_eq!(result, Ok(IsFile(true))); // Write to a missing file. let result = tester.test_redirection_target(L!("newfile.txt"), RedirectionMode::Overwrite); - assert!(result); + assert_eq!(result, Ok(IsFile(false))); // No-clobber write to existing file should fail. let result = tester.test_redirection_target(L!("file.txt"), RedirectionMode::NoClob); - assert!(!result); + assert_eq!(result, Err(IsErr)); // No-clobber write to missing file should succeed. let result = tester.test_redirection_target(L!("unique.txt"), RedirectionMode::NoClob); - assert!(result); + assert_eq!(result, Ok(IsFile(false))); let write_modes = &[ RedirectionMode::Overwrite, @@ -606,8 +613,9 @@ fn test_redirections() { // Can't write to a directory. for mode in write_modes { - assert!( - !tester.test_redirection_target(L!("somedir"), *mode), + assert_eq!( + tester.test_redirection_target(L!("somedir"), *mode), + Err(IsErr), "Should not be able to write to a directory with mode {:?}", mode ); @@ -616,8 +624,9 @@ fn test_redirections() { // Can't write without write permissions. fs::set_permissions(&file_path, Permissions::from_mode(0o400)).unwrap(); // Read-only. for mode in write_modes { - assert!( - !tester.test_redirection_target(L!("file.txt"), *mode), + assert_eq!( + tester.test_redirection_target(L!("file.txt"), *mode), + Err(IsErr), "Should not be able to write to a read-only file with mode {:?}", mode ); @@ -629,8 +638,9 @@ fn test_redirections() { { fs::set_permissions(&dir_path, Permissions::from_mode(0o500)).unwrap(); // Read and execute, no write. for mode in write_modes { - assert!( - !tester.test_redirection_target(L!("somedir/newfile.txt"), *mode), + assert_eq!( + tester.test_redirection_target(L!("somedir/newfile.txt"), *mode), + Err(IsErr), "Should not be able to create/write in a read-only directory with mode {:?}", mode ); @@ -639,28 +649,82 @@ fn test_redirections() { } // Test fd redirections. - assert!(tester.test_redirection_target(L!("-"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("0"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("1"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("2"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("3"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("500"), RedirectionMode::Fd)); + assert_eq!( + tester.test_redirection_target(L!("-"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("0"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("1"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("2"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("3"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("500"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); // We are base 10, despite the leading 0. - assert!(tester.test_redirection_target(L!("000"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("01"), RedirectionMode::Fd)); - assert!(tester.test_redirection_target(L!("07"), RedirectionMode::Fd)); + assert_eq!( + tester.test_redirection_target(L!("000"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("01"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); + assert_eq!( + tester.test_redirection_target(L!("07"), RedirectionMode::Fd), + Ok(IsFile(false)), + ); // Invalid fd redirections. - assert!(!tester.test_redirection_target(L!("0x2"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("0x3F"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("0F"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("-1"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("-0009"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("--"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("derp"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("123boo"), RedirectionMode::Fd)); - assert!(!tester.test_redirection_target(L!("18446744073709551616"), RedirectionMode::Fd)); + assert_eq!( + tester.test_redirection_target(L!("0x2"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("0x3F"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("0F"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("-1"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("-0009"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("--"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("derp"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("123boo"), RedirectionMode::Fd), + Err(IsErr), + ); + assert_eq!( + tester.test_redirection_target(L!("18446744073709551616"), RedirectionMode::Fd), + Err(IsErr), + ); } #[test] diff --git a/src/highlight/highlight.rs b/src/highlight/highlight.rs index 0d0aa72f4..8387559fe 100644 --- a/src/highlight/highlight.rs +++ b/src/highlight/highlight.rs @@ -967,25 +967,25 @@ fn visit_redirection(&mut self, redir: &Redirection) { } // No command substitution, so we can highlight the target file or fd. For example, // disallow redirections into a non-existent directory. - let target_is_valid = if !self.io_still_ok() { + let (role, file_exists) = if !self.io_still_ok() { // I/O is disallowed, so we don't have much hope of catching anything but gross // errors. Assume it's valid. - true + (HighlightRole::redirection, false) } else if contains_pending_variable(&self.pending_variables, &target) { - true + // Target uses a variable defined by the current commandline. Assume it's valid. + (HighlightRole::redirection, false) } else { // Validate the redirection target.. - self.file_tester.test_redirection_target(&target, oper.mode) - }; - self.color_node( - &redir.target, - HighlightSpec::with_fg(if target_is_valid { - HighlightRole::redirection + if let Ok(IsFile(file_exists)) = + self.file_tester.test_redirection_target(&target, oper.mode) + { + (HighlightRole::redirection, file_exists) } else { - HighlightRole::error - }), - ); - if target_is_valid && self.io_still_ok() && self.file_tester.test_path(&target, false) { + (HighlightRole::error, false) + } + }; + self.color_node(&redir.target, HighlightSpec::with_fg(role)); + if file_exists { for i in redir.target.source_range().as_usize() { self.color_array[i].valid_path = true; }