From f39487fca9301c45eaf50af437a269d3c0600f61 Mon Sep 17 00:00:00 2001 From: Ethan P Date: Mon, 11 May 2020 19:22:13 -0700 Subject: [PATCH 1/9] Make syntax detection more consistent for Reader and File inputs --- src/assets.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/assets.rs b/src/assets.rs index 4f8556f0..58ac2c08 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -218,7 +218,7 @@ impl HighlightingAssets { } } } - OpenedInputKind::StdIn | OpenedInputKind::CustomReader => { + OpenedInputKind::StdIn => { if let Some(ref name) = input.metadata.user_provided_name { self.get_extension_syntax(&name) .or_else(|| self.get_first_line_syntax(&mut input.reader)) @@ -226,6 +226,29 @@ impl HighlightingAssets { self.get_first_line_syntax(&mut input.reader) } } + OpenedInputKind::CustomReader => { + if let Some(ref path_str) = input.metadata.user_provided_name { + let path = Path::new(path_str); + let absolute_path = + path.canonicalize().ok().unwrap_or_else(|| path.to_owned()); + let line_syntax = self.get_first_line_syntax(&mut input.reader); + + match mapping.get_syntax_for(path_str) { + Some(MappingTarget::MapTo(syntax_name)) => { + println!("Mapped {:?} as {:?}", path_str, syntax_name); + self.syntax_set.find_syntax_by_name(syntax_name) + } + Some(MappingTarget::MapToUnknown) => line_syntax, + None => { + println!("Test {:?}", path_str); + let file_name = path.file_name().unwrap_or_default(); + self.get_extension_syntax(file_name).or(line_syntax) + } + } + } else { + self.get_first_line_syntax(&mut input.reader) + } + } OpenedInputKind::ThemePreviewFile => self.syntax_set.find_syntax_by_name("Rust"), } }; From 7d07aa395a90f33bb939625de8576c0fa667aef7 Mon Sep 17 00:00:00 2001 From: Ethan P Date: Mon, 11 May 2020 19:23:56 -0700 Subject: [PATCH 2/9] Change assets.rs tests to use InputKind::CustomReader This should avoid errors in filesystems that don't support UTF-8 or invalid UTF-8. --- src/assets.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/assets.rs b/src/assets.rs index 58ac2c08..6cf2e2c4 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -233,14 +233,12 @@ impl HighlightingAssets { path.canonicalize().ok().unwrap_or_else(|| path.to_owned()); let line_syntax = self.get_first_line_syntax(&mut input.reader); - match mapping.get_syntax_for(path_str) { + match mapping.get_syntax_for(absolute_path) { Some(MappingTarget::MapTo(syntax_name)) => { - println!("Mapped {:?} as {:?}", path_str, syntax_name); self.syntax_set.find_syntax_by_name(syntax_name) } Some(MappingTarget::MapToUnknown) => line_syntax, None => { - println!("Test {:?}", path_str); let file_name = path.file_name().unwrap_or_default(); self.get_extension_syntax(file_name).or(line_syntax) } @@ -281,8 +279,6 @@ mod tests { use super::*; use std::ffi::OsStr; - use std::fs::File; - use std::io::Write; use tempdir::TempDir; @@ -306,12 +302,8 @@ mod tests { fn syntax_for_file_with_content_os(&self, file_name: &OsStr, first_line: &str) -> String { let file_path = self.temp_dir.path().join(file_name); - { - let mut temp_file = File::create(&file_path).unwrap(); - writeln!(temp_file, "{}", first_line).unwrap(); - } - - let input = Input::ordinary_file(file_path.as_os_str()); + let input = Input::from_reader(Box::new(BufReader::new(first_line.as_bytes()))) + .with_name(Some(file_path.as_os_str())); let dummy_stdin: &[u8] = &[]; let mut opened_input = input.open(dummy_stdin).unwrap(); let syntax = self From 1fb669ae1abfff0712dc370133ae92af56aa439c Mon Sep 17 00:00:00 2001 From: Ethan P Date: Mon, 11 May 2020 20:10:04 -0700 Subject: [PATCH 3/9] Test that OrdinaryFile consistent with CustomReader --- src/assets.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/assets.rs b/src/assets.rs index 6cf2e2c4..364e17bc 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -280,6 +280,8 @@ mod tests { use std::ffi::OsStr; + use std::fs::File; + use std::io::Write; use tempdir::TempDir; use crate::input::Input; @@ -300,6 +302,27 @@ mod tests { } } + fn syntax_for_real_file_with_content_os( + &self, + file_name: &OsStr, + first_line: &str, + ) -> String { + let file_path = self.temp_dir.path().join(file_name); + { + let mut temp_file = File::create(&file_path).unwrap(); + writeln!(temp_file, "{}", first_line).unwrap(); + } + + let input = Input::ordinary_file(file_path.as_os_str()); + let dummy_stdin: &[u8] = &[]; + let mut opened_input = input.open(dummy_stdin).unwrap(); + let syntax = self + .assets + .get_syntax(None, &mut opened_input, &self.syntax_mapping); + + syntax.name.clone() + } + fn syntax_for_file_with_content_os(&self, file_name: &OsStr, first_line: &str) -> String { let file_path = self.temp_dir.path().join(file_name); let input = Input::from_reader(Box::new(BufReader::new(first_line.as_bytes()))) @@ -334,6 +357,12 @@ mod tests { .get_syntax(None, &mut opened_input, &self.syntax_mapping); syntax.name.clone() } + + fn syntax_is_same_for_file_and_reader(&self, file_name: &str, content: &str) -> bool { + let real = self.syntax_for_real_file_with_content_os(file_name.as_ref(), content); + let fake = self.syntax_for_file_with_content_os(file_name.as_ref(), content); + return real == fake; + } } #[test] @@ -364,6 +393,27 @@ mod tests { ); } + #[test] + fn syntax_detection_same_for_file_and_string() { + let mut test = SyntaxDetectionTest::new(); + + test.syntax_mapping + .insert("*.myext", MappingTarget::MapTo("C")) + .ok(); + test.syntax_mapping + .insert("MY_FILE", MappingTarget::MapTo("Markdown")) + .ok(); + + assert!(test.syntax_is_same_for_file_and_reader("Test.md", "")); + assert!(test.syntax_is_same_for_file_and_reader("Test.txt", "#!/bin/bash")); + assert!(test.syntax_is_same_for_file_and_reader(".bashrc", "")); + assert!(test.syntax_is_same_for_file_and_reader("test.h", "")); + assert!(test.syntax_is_same_for_file_and_reader("test.js", "#!/bin/bash")); + assert!(test.syntax_is_same_for_file_and_reader("test.myext", "")); + assert!(test.syntax_is_same_for_file_and_reader("MY_FILE", "")); + assert!(test.syntax_is_same_for_file_and_reader("MY_FILE", " Date: Wed, 13 May 2020 00:47:18 -0700 Subject: [PATCH 4/9] Rename test for checking if inputkinds are consistent --- src/assets.rs | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/assets.rs b/src/assets.rs index 364e17bc..00a1c2f6 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -358,10 +358,20 @@ mod tests { syntax.name.clone() } - fn syntax_is_same_for_file_and_reader(&self, file_name: &str, content: &str) -> bool { - let real = self.syntax_for_real_file_with_content_os(file_name.as_ref(), content); - let fake = self.syntax_for_file_with_content_os(file_name.as_ref(), content); - return real == fake; + fn syntax_is_same_for_inputkinds(&self, file_name: &str, content: &str) -> bool { + let as_file = self.syntax_for_real_file_with_content_os(file_name.as_ref(), content); + let as_reader = self.syntax_for_file_with_content_os(file_name.as_ref(), content); + let consistent = as_file == as_reader; + // TODO: Compare StdIn somehow? + + if !consistent { + eprintln!( + "Inconsistent syntax detection:\nFor File: {}\nFor Reader: {}", + as_file, as_reader + ) + } + + consistent } } @@ -394,7 +404,7 @@ mod tests { } #[test] - fn syntax_detection_same_for_file_and_string() { + fn syntax_detection_same_for_inputkinds() { let mut test = SyntaxDetectionTest::new(); test.syntax_mapping @@ -404,14 +414,14 @@ mod tests { .insert("MY_FILE", MappingTarget::MapTo("Markdown")) .ok(); - assert!(test.syntax_is_same_for_file_and_reader("Test.md", "")); - assert!(test.syntax_is_same_for_file_and_reader("Test.txt", "#!/bin/bash")); - assert!(test.syntax_is_same_for_file_and_reader(".bashrc", "")); - assert!(test.syntax_is_same_for_file_and_reader("test.h", "")); - assert!(test.syntax_is_same_for_file_and_reader("test.js", "#!/bin/bash")); - assert!(test.syntax_is_same_for_file_and_reader("test.myext", "")); - assert!(test.syntax_is_same_for_file_and_reader("MY_FILE", "")); - assert!(test.syntax_is_same_for_file_and_reader("MY_FILE", " Date: Wed, 13 May 2020 02:51:49 -0700 Subject: [PATCH 5/9] Add regression test for detected syntax differing for stdin and files --- tests/examples/regression_tests/issue_985.js | 5 +++ tests/integration_tests.rs | 39 ++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/examples/regression_tests/issue_985.js diff --git a/tests/examples/regression_tests/issue_985.js b/tests/examples/regression_tests/issue_985.js new file mode 100644 index 00000000..5e2e2191 --- /dev/null +++ b/tests/examples/regression_tests/issue_985.js @@ -0,0 +1,5 @@ +// This test should be considered a failure if the detected syntax differs between the following two commands: +/* +# bat --map-syntax '*.js:Markdown' --file-name 'issue_985.js' < issue_985.js +# bat --map-syntax '*.js:Markdown' --file-name 'issue_985.js' issue_985.js +*/ diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 89c15832..6e49cff6 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -1,4 +1,8 @@ use assert_cmd::Command; +use std::path::Path; +use std::str::from_utf8; + +const EXAMPLES_DIR: &str = "tests/examples"; fn bat_with_config() -> Command { let mut cmd = Command::cargo_bin("bat").unwrap(); @@ -670,3 +674,38 @@ fn do_not_panic_regression_tests() { .success(); } } + +#[test] +fn do_not_detect_different_syntax_for_stdin_and_files() { + let file = "regression_tests/issue_985.js"; + + let output_for_file = bat() + .arg("--color=always") + .arg("--map-syntax=*.js:Markdown") + .arg(&format!("--file-name={}", file)) + .arg("--style=plain") + .arg(file) + .assert() + .success() + .get_output() + .stdout + .clone(); + + let output_for_stdin = bat() + .arg("--color=always") + .arg("--map-syntax=*.js:Markdown") + .arg("--style=plain") + .arg(&format!("--file-name={}", file)) + .pipe_stdin(Path::new(EXAMPLES_DIR).join(file)) + .unwrap() + .assert() + .success() + .get_output() + .stdout + .clone(); + + assert_eq!( + from_utf8(&output_for_file).unwrap(), + from_utf8(&output_for_stdin).unwrap() + ); +} From 59140b458c98fd9585b6c7207a106d73bbbb39f1 Mon Sep 17 00:00:00 2001 From: Ethan P Date: Wed, 13 May 2020 01:56:19 -0700 Subject: [PATCH 6/9] Consolidate syntax detection behavior for all InputKind types --- src/assets.rs | 89 ++++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 51 deletions(-) diff --git a/src/assets.rs b/src/assets.rs index 00a1c2f6..0448df09 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -191,63 +191,50 @@ impl HighlightingAssets { input: &mut OpenedInput, mapping: &SyntaxMapping, ) -> &SyntaxReference { - let syntax = if let Some(language) = language { + let syntax = if match input.kind { + OpenedInputKind::ThemePreviewFile => true, + _ => false, + } { + // FIXME: replace the above match statement with matches macro when min Rust >= 1.42.0 + self.syntax_set.find_syntax_by_name("Rust") + } else if let Some(language) = language { self.syntax_set.find_syntax_by_token(language) } else { - match input.kind { - OpenedInputKind::OrdinaryFile(ref actual_path) => { - let path_str = input - .metadata - .user_provided_name - .as_ref() - .unwrap_or(actual_path); - let path = Path::new(path_str); - let line_syntax = self.get_first_line_syntax(&mut input.reader); + let line_syntax = self.get_first_line_syntax(&mut input.reader); - let absolute_path = path.canonicalize().ok().unwrap_or_else(|| path.to_owned()); - match mapping.get_syntax_for(absolute_path) { - Some(MappingTarget::MapTo(syntax_name)) => { - // TODO: we should probably return an error here if this syntax can not be - // found. Currently, we just fall back to 'plain'. - self.syntax_set.find_syntax_by_name(syntax_name) - } - Some(MappingTarget::MapToUnknown) => line_syntax, - None => { - let file_name = path.file_name().unwrap_or_default(); - self.get_extension_syntax(file_name).or(line_syntax) - } - } - } - OpenedInputKind::StdIn => { - if let Some(ref name) = input.metadata.user_provided_name { - self.get_extension_syntax(&name) - .or_else(|| self.get_first_line_syntax(&mut input.reader)) - } else { - self.get_first_line_syntax(&mut input.reader) - } - } - OpenedInputKind::CustomReader => { - if let Some(ref path_str) = input.metadata.user_provided_name { - let path = Path::new(path_str); - let absolute_path = - path.canonicalize().ok().unwrap_or_else(|| path.to_owned()); - let line_syntax = self.get_first_line_syntax(&mut input.reader); + // Get the path of the file: + // If this was set by the metadata, that will take priority. + // If it wasn't, it will use the real file path (if available). + let path_str = + input + .metadata + .user_provided_name + .as_ref() + .or_else(|| match input.kind { + OpenedInputKind::OrdinaryFile(ref path) => Some(path), + _ => None, + }); - match mapping.get_syntax_for(absolute_path) { - Some(MappingTarget::MapTo(syntax_name)) => { - self.syntax_set.find_syntax_by_name(syntax_name) - } - Some(MappingTarget::MapToUnknown) => line_syntax, - None => { - let file_name = path.file_name().unwrap_or_default(); - self.get_extension_syntax(file_name).or(line_syntax) - } - } - } else { - self.get_first_line_syntax(&mut input.reader) + if let Some(path_str) = path_str { + // If a path was provided, we try and detect the syntax based on extension mappings. + let path = Path::new(path_str); + let absolute_path = path.canonicalize().ok().unwrap_or_else(|| path.to_owned()); + + match mapping.get_syntax_for(absolute_path) { + Some(MappingTarget::MapToUnknown) => line_syntax, + Some(MappingTarget::MapTo(syntax_name)) => { + // TODO: we should probably return an error here if this syntax can not be + // found. Currently, we just fall back to 'plain'. + self.syntax_set.find_syntax_by_name(syntax_name) + } + None => { + let file_name = path.file_name().unwrap_or_default(); + self.get_extension_syntax(file_name).or(line_syntax) } } - OpenedInputKind::ThemePreviewFile => self.syntax_set.find_syntax_by_name("Rust"), + } else { + // If a path wasn't provided, we fall back to the detect first-line syntax. + line_syntax } }; From cc52f79e421d8f4e18600605340cdeee7ecd6c02 Mon Sep 17 00:00:00 2001 From: Ethan P Date: Fri, 15 May 2020 12:19:11 -0700 Subject: [PATCH 7/9] Add helper fn for checking if opened input is theme preview file --- src/assets.rs | 6 +----- src/input.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/assets.rs b/src/assets.rs index 0448df09..7a3cc81f 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -191,11 +191,7 @@ impl HighlightingAssets { input: &mut OpenedInput, mapping: &SyntaxMapping, ) -> &SyntaxReference { - let syntax = if match input.kind { - OpenedInputKind::ThemePreviewFile => true, - _ => false, - } { - // FIXME: replace the above match statement with matches macro when min Rust >= 1.42.0 + let syntax = if input.kind.is_theme_preview_file() { self.syntax_set.find_syntax_by_name("Rust") } else if let Some(language) = language { self.syntax_set.find_syntax_by_token(language) diff --git a/src/input.rs b/src/input.rs index 929a6524..eeb9a494 100644 --- a/src/input.rs +++ b/src/input.rs @@ -39,6 +39,15 @@ pub(crate) enum OpenedInputKind { CustomReader, } +impl OpenedInputKind { + pub(crate) fn is_theme_preview_file(&self) -> bool { + match self { + OpenedInputKind::ThemePreviewFile => true, + _ => false, + } + } +} + pub(crate) struct OpenedInput<'a> { pub(crate) kind: OpenedInputKind, pub(crate) metadata: InputMetadata, From 9166c9dd355700067adb117382d6246bdefb8c7d Mon Sep 17 00:00:00 2001 From: Ethan P Date: Fri, 15 May 2020 12:29:01 -0700 Subject: [PATCH 8/9] Remove unnecessary clone in integration tests --- tests/integration_tests.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 6e49cff6..36954b3d 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -679,19 +679,16 @@ fn do_not_panic_regression_tests() { fn do_not_detect_different_syntax_for_stdin_and_files() { let file = "regression_tests/issue_985.js"; - let output_for_file = bat() + let cmd_for_file = bat() .arg("--color=always") .arg("--map-syntax=*.js:Markdown") .arg(&format!("--file-name={}", file)) .arg("--style=plain") .arg(file) .assert() - .success() - .get_output() - .stdout - .clone(); + .success(); - let output_for_stdin = bat() + let cmd_for_stdin = bat() .arg("--color=always") .arg("--map-syntax=*.js:Markdown") .arg("--style=plain") @@ -699,13 +696,10 @@ fn do_not_detect_different_syntax_for_stdin_and_files() { .pipe_stdin(Path::new(EXAMPLES_DIR).join(file)) .unwrap() .assert() - .success() - .get_output() - .stdout - .clone(); + .success(); assert_eq!( - from_utf8(&output_for_file).unwrap(), - from_utf8(&output_for_stdin).unwrap() + from_utf8(&cmd_for_file.get_output().stdout).expect("output is valid utf-8"), + from_utf8(&cmd_for_stdin.get_output().stdout).expect("output is valid utf-8") ); } From 403ab6e44398ca081e5c72c6978904e7bdc4ba9c Mon Sep 17 00:00:00 2001 From: Ethan P Date: Fri, 15 May 2020 13:23:24 -0700 Subject: [PATCH 9/9] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fca99cb4..69aae01d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ## Bugfixes - bat mishighlights Users that start with digits in SSH config, see #984 +- `--map-syntax` doesn't work with names provided through `--file-name` (@eth-p) ## Other ## New syntaxes