Skip to content

Commit

Permalink
gitignore: add more detail to errors on invalid pattern
Browse files Browse the repository at this point in the history
  • Loading branch information
@scott2000
scott2000 committed Dec 17, 2024
1 parent 7f1d902 commit 9591523
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 30 deletions.
6 changes: 5 additions & 1 deletion cli/examples/custom-working-copy/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,11 @@ impl LockedWorkingCopy for LockedConflictsWorkingCopy {
options: &SnapshotOptions,
) -> Result<(MergedTreeId, SnapshotStats), SnapshotError> {
let options = SnapshotOptions {
base_ignores: options.base_ignores.chain("", "/.conflicts".as_bytes())?,
base_ignores: options.base_ignores.chain(
"",
Path::new(""),
"/.conflicts".as_bytes(),
)?,
..options.clone()
};
self.inner.snapshot(&options)
Expand Down
6 changes: 4 additions & 2 deletions cli/tests/test_working_copy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,15 +239,17 @@ fn test_snapshot_invalid_ignore_pattern() {
std::fs::write(&gitignore_path, " []\n").unwrap();
insta::assert_snapshot!(test_env.jj_cmd_internal_error(&repo_path, &["st"]), @r#"
Internal error: Failed to snapshot the working copy
Caused by: error parsing glob ' []': unclosed character class; missing ']'
Caused by:
1: Failed to parse ignore patterns from file $TEST_ENV/repo/.gitignore
2: error parsing glob ' []': unclosed character class; missing ']'
"#);

// Test invalid UTF-8 in .gitignore
std::fs::write(&gitignore_path, b"\xff\n").unwrap();
insta::assert_snapshot!(test_env.jj_cmd_internal_error(&repo_path, &["st"]), @r##"
Internal error: Failed to snapshot the working copy
Caused by:
1: invalid UTF-8 for ignore pattern in on line #1: �
1: Invalid UTF-8 for ignore pattern in $TEST_ENV/repo/.gitignore on line #1: �
2: invalid utf-8 sequence of 1 bytes from index 0
"##);
}
89 changes: 62 additions & 27 deletions lib/src/gitignore.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use std::fs;
use std::io;
use std::iter;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;

Expand All @@ -27,15 +28,18 @@ use thiserror::Error;
pub enum GitIgnoreError {
#[error("Failed to read ignore patterns from file {path}")]
ReadFile { path: PathBuf, source: io::Error },
#[error("invalid UTF-8 for ignore pattern in {path} on line #{line_num_for_display}: {line}")]
#[error("Invalid UTF-8 for ignore pattern in {path} on line #{line_num_for_display}: {line}")]
InvalidUtf8 {
path: PathBuf,
line_num_for_display: usize,
line: String,
source: std::str::Utf8Error,
},
#[error(transparent)]
Underlying(#[from] ignore::Error),
#[error("Failed to parse ignore patterns from file {path}")]
Underlying {
path: PathBuf,
source: ignore::Error,
},
}

/// Models the effective contents of multiple .gitignore files.
Expand All @@ -60,22 +64,31 @@ impl GitIgnoreFile {
pub fn chain(
self: &Arc<GitIgnoreFile>,
prefix: &str,
ignore_path: &Path,
input: &[u8],
) -> Result<Arc<GitIgnoreFile>, GitIgnoreError> {
let mut builder = gitignore::GitignoreBuilder::new(prefix);
for (i, input_line) in input.split(|b| *b == b'\n').enumerate() {
let line =
std::str::from_utf8(input_line).map_err(|err| GitIgnoreError::InvalidUtf8 {
path: PathBuf::from(prefix),
path: ignore_path.to_path_buf(),
line_num_for_display: i + 1,
line: String::from_utf8_lossy(input_line).to_string(),
source: err,
})?;
// FIXME: do we need to provide the `from` argument? Is it for providing
// diagnostics or correctness?
builder.add_line(None, line)?;
builder
.add_line(None, line)
.map_err(|err| GitIgnoreError::Underlying {
path: ignore_path.to_path_buf(),
source: err,
})?;
}
let matcher = builder.build()?;
let matcher = builder.build().map_err(|err| GitIgnoreError::Underlying {
path: ignore_path.to_path_buf(),
source: err,
})?;
let parent = if self.matcher.is_empty() {
self.parent.clone() // omit the empty root
} else {
Expand All @@ -98,7 +111,7 @@ impl GitIgnoreFile {
path: file.clone(),
source: err,
})?;
self.chain(prefix, &buf)
self.chain(prefix, &file, &buf)
} else {
Ok(self.clone())
}
Expand Down Expand Up @@ -142,7 +155,9 @@ mod tests {
use super::*;

fn matches(input: &[u8], path: &str) -> bool {
let file = GitIgnoreFile::empty().chain("", input).unwrap();
let file = GitIgnoreFile::empty()
.chain("", Path::new(""), input)
.unwrap();
file.matches(path)
}

Expand All @@ -154,13 +169,17 @@ mod tests {

#[test]
fn test_gitignore_empty_file_with_prefix() {
let file = GitIgnoreFile::empty().chain("dir/", b"").unwrap();
let file = GitIgnoreFile::empty()
.chain("dir/", Path::new(""), b"")
.unwrap();
assert!(!file.matches("dir/foo"));
}

#[test]
fn test_gitignore_literal() {
let file = GitIgnoreFile::empty().chain("", b"foo\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("", Path::new(""), b"foo\n")
.unwrap();
assert!(file.matches("foo"));
assert!(file.matches("dir/foo"));
assert!(file.matches("dir/subdir/foo"));
Expand All @@ -170,37 +189,45 @@ mod tests {

#[test]
fn test_gitignore_literal_with_prefix() {
let file = GitIgnoreFile::empty().chain("./dir/", b"foo\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("./dir/", Path::new(""), b"foo\n")
.unwrap();
assert!(file.matches("dir/foo"));
assert!(file.matches("dir/subdir/foo"));
}

#[test]
fn test_gitignore_pattern_same_as_prefix() {
let file = GitIgnoreFile::empty().chain("dir/", b"dir\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("dir/", Path::new(""), b"dir\n")
.unwrap();
assert!(file.matches("dir/dir"));
// We don't want the "dir" pattern to apply to the parent directory
assert!(!file.matches("dir/foo"));
}

#[test]
fn test_gitignore_rooted_literal() {
let file = GitIgnoreFile::empty().chain("", b"/foo\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("", Path::new(""), b"/foo\n")
.unwrap();
assert!(file.matches("foo"));
assert!(!file.matches("dir/foo"));
}

#[test]
fn test_gitignore_rooted_literal_with_prefix() {
let file = GitIgnoreFile::empty().chain("dir/", b"/foo\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("dir/", Path::new(""), b"/foo\n")
.unwrap();
assert!(file.matches("dir/foo"));
assert!(!file.matches("dir/subdir/foo"));
}

#[test]
fn test_gitignore_deep_dir() {
let file = GitIgnoreFile::empty()
.chain("", b"/dir1/dir2/dir3\n")
.chain("", Path::new(""), b"/dir1/dir2/dir3\n")
.unwrap();
assert!(!file.matches("foo"));
assert!(!file.matches("dir1/foo"));
Expand All @@ -213,11 +240,11 @@ mod tests {
fn test_gitignore_deep_dir_chained() {
// Prefix is relative to root, not to parent file
let file = GitIgnoreFile::empty()
.chain("", b"/dummy\n")
.chain("", Path::new(""), b"/dummy\n")
.unwrap()
.chain("dir1/", b"/dummy\n")
.chain("dir1/", Path::new(""), b"/dummy\n")
.unwrap()
.chain("dir1/dir2/", b"/dir3\n")
.chain("dir1/dir2/", Path::new(""), b"/dir3\n")
.unwrap();
assert!(!file.matches("foo"));
assert!(!file.matches("dir1/foo"));
Expand All @@ -228,7 +255,9 @@ mod tests {

#[test]
fn test_gitignore_match_only_dir() {
let file = GitIgnoreFile::empty().chain("", b"/dir/\n").unwrap();
let file = GitIgnoreFile::empty()
.chain("", Path::new(""), b"/dir/\n")
.unwrap();
assert!(!file.matches("dir"));
assert!(file.matches("dir/foo"));
assert!(file.matches("dir/subdir/foo"));
Expand All @@ -242,7 +271,9 @@ mod tests {
assert!(matches(b"\\?\n", "?"));
assert!(!matches(b"\\?\n", "x"));
assert!(matches(b"\\w\n", "w"));
assert!(GitIgnoreFile::empty().chain("", b"\\\n").is_err());
assert!(GitIgnoreFile::empty()
.chain("", Path::new(""), b"\\\n")
.is_err());
}

#[test]
Expand Down Expand Up @@ -295,7 +326,9 @@ mod tests {
assert!(matches(b"a\r\r\n", "a"));
assert!(matches(b"\ra\n", "\ra"));
assert!(!matches(b"\ra\n", "a"));
assert!(GitIgnoreFile::empty().chain("", b"a b \\ \n").is_err());
assert!(GitIgnoreFile::empty()
.chain("", Path::new(""), b"a b \\ \n")
.is_err());
}

#[test]
Expand Down Expand Up @@ -335,7 +368,7 @@ mod tests {
#[test]
fn test_gitignore_leading_dir_glob_with_prefix() {
let file = GitIgnoreFile::empty()
.chain("dir1/dir2/", b"**/foo\n")
.chain("dir1/dir2/", Path::new(""), b"**/foo\n")
.unwrap();
assert!(file.matches("dir1/dir2/foo"));
assert!(!file.matches("dir1/dir2/bar"));
Expand Down Expand Up @@ -381,9 +414,11 @@ mod tests {

#[test]
fn test_gitignore_file_ordering() {
let file1 = GitIgnoreFile::empty().chain("", b"/foo\n").unwrap();
let file2 = file1.chain("foo/", b"!/bar").unwrap();
let file3 = file2.chain("foo/bar/", b"/baz").unwrap();
let file1 = GitIgnoreFile::empty()
.chain("", Path::new(""), b"/foo\n")
.unwrap();
let file2 = file1.chain("foo/", Path::new(""), b"!/bar").unwrap();
let file3 = file2.chain("foo/bar/", Path::new(""), b"/baz").unwrap();
assert!(file1.matches("foo"));
assert!(file1.matches("foo/bar"));
assert!(!file2.matches("foo/bar"));
Expand All @@ -408,12 +443,12 @@ mod tests {
// A/B.ext
// ```
let ignore = GitIgnoreFile::empty()
.chain("", b"foo/bar.*\n!/foo/\n")
.chain("", Path::new(""), b"foo/bar.*\n!/foo/\n")
.unwrap();
assert!(ignore.matches("foo/bar.ext"));

let ignore = GitIgnoreFile::empty()
.chain("", b"!/foo/\nfoo/bar.*\n")
.chain("", Path::new(""), b"!/foo/\nfoo/bar.*\n")
.unwrap();
assert!(ignore.matches("foo/bar.ext"));
}
Expand Down

0 comments on commit 9591523

Please sign in to comment.