From 2109a7b4884e5d6b99311bc9b7886d71355ddee1 Mon Sep 17 00:00:00 2001 From: Piotr Kufel Date: Thu, 17 Aug 2023 16:48:33 -0700 Subject: [PATCH] Fix .gitignore handling of ignored directories - Ignore .gitignore files from untracked directories - Do not allow un-ignoring files within ignored directories --- CHANGELOG.md | 3 + cli/tests/test_status_command.rs | 25 +++ lib/src/gitignore.rs | 253 ++++++++++++++----------------- lib/src/working_copy.rs | 4 +- 4 files changed, 143 insertions(+), 142 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77a12e72a..acbd3409f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs +* Fix issues related to .gitignore handling of untracked directories + [#2051](https://github.com/martinvonz/jj/issues/2051). + * `jj config set --user` and `jj config edit --user` can now be used outside of any repository. * SSH authentication could hang when ssh-agent couldn't be reached diff --git a/cli/tests/test_status_command.rs b/cli/tests/test_status_command.rs index 59c30c598..8603f262e 100644 --- a/cli/tests/test_status_command.rs +++ b/cli/tests/test_status_command.rs @@ -39,3 +39,28 @@ fn test_status_merge() { Parent commit: zsuskuln 29b991e9 right "###); } + +// See https://github.com/martinvonz/jj/issues/2051. +#[test] +fn test_status_ignored_gitignore() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::create_dir(repo_path.join("untracked")).unwrap(); + std::fs::write(repo_path.join("untracked").join("inside_untracked"), "test").unwrap(); + std::fs::write( + repo_path.join("untracked").join(".gitignore"), + "!inside_untracked\n", + ) + .unwrap(); + std::fs::write(repo_path.join(".gitignore"), "untracked/\n!dummy\n").unwrap(); + + let stdout = test_env.jj_cmd_success(&repo_path, &["status"]); + insta::assert_snapshot!(stdout, @r###" + Working copy changes: + A .gitignore + Working copy : qpvuntsm 88a40909 (no description set) + Parent commit: zzzzzzzz 00000000 (empty) (no description set) + "###); +} diff --git a/lib/src/gitignore.rs b/lib/src/gitignore.rs index d6143e93e..a6ecebc75 100644 --- a/lib/src/gitignore.rs +++ b/lib/src/gitignore.rs @@ -146,6 +146,7 @@ impl GitIgnoreLine { } } +/// Models the effective contents of multiple .gitignore files. #[derive(Debug)] pub struct GitIgnoreFile { parent: Option>, @@ -198,7 +199,14 @@ impl GitIgnoreFile { } } - pub fn matches_file(&self, path: &str) -> bool { + /// Returns whether specified path (not just file!) should be ignored. This + /// method does not directly define which files should not be tracked in + /// the repository. Instead, it performs a simple matching against the + /// last applicable .gitignore line. The effective set of paths + /// ignored in the repository should take into account that all (untracked) + /// files within a ignored directory should be ignored unconditionally. + /// The code in this file does not take that into account. + pub fn matches(&self, path: &str) -> bool { // Later lines take precedence, so check them in reverse for line in self.all_lines_reversed() { if line.matches(path) { @@ -207,26 +215,6 @@ impl GitIgnoreFile { } false } - - pub fn matches_all_files_in(&self, dir: &str) -> bool { - // Later lines take precedence, so check them in reverse - assert!(dir.is_empty() || dir.ends_with('/')); - for line in self.all_lines_reversed() { - // Let's say there's a "/target/" pattern and then a "!interesting" pattern - // after it, then we can't say for sure that all files in target/ match. - // TODO: This can be smarter. For example, if there's a pattern "/foo/" followed - // by "!/bar/", then we can answer "true" for "foo/". A more complex - // case is if a pattern "/foo/" is followed "!/foo/bar/", then we - // can say "false" for "foo/" and "true" for "foo/baz/". - if line.is_negative { - return false; - } - if line.matches(dir) { - return true; - } - } - false - } } #[cfg(test)] @@ -234,36 +222,31 @@ mod tests { use super::*; - fn matches_file(input: &[u8], path: &str) -> bool { + fn matches(input: &[u8], path: &str) -> bool { let file = GitIgnoreFile::empty().chain("", input); - file.matches_file(path) - } - - fn matches_all_files_in(input: &[u8], path: &str) -> bool { - let file = GitIgnoreFile::empty().chain("", input); - file.matches_all_files_in(path) + file.matches(path) } #[test] fn test_gitignore_empty_file() { let file = GitIgnoreFile::empty(); - assert!(!file.matches_file("foo")); + assert!(!file.matches("foo")); } #[test] fn test_gitignore_empty_file_with_prefix() { let file = GitIgnoreFile::empty().chain("dir/", b""); - assert!(!file.matches_file("dir/foo")); + assert!(!file.matches("dir/foo")); } #[test] fn test_gitignore_literal() { let file = GitIgnoreFile::empty().chain("", b"foo\n"); - assert!(file.matches_file("foo")); - assert!(file.matches_file("dir/foo")); - assert!(file.matches_file("dir/subdir/foo")); - assert!(!file.matches_file("food")); - assert!(!file.matches_file("dir/food")); + assert!(file.matches("foo")); + assert!(file.matches("dir/foo")); + assert!(file.matches("dir/subdir/foo")); + assert!(!file.matches("food")); + assert!(!file.matches("dir/food")); } #[test] @@ -271,24 +254,24 @@ mod tests { let file = GitIgnoreFile::empty().chain("dir/", b"foo\n"); // I consider it undefined whether a file in a parent directory matches, but // let's test it anyway - assert!(!file.matches_file("foo")); - assert!(file.matches_file("dir/foo")); - assert!(file.matches_file("dir/subdir/foo")); + assert!(!file.matches("foo")); + 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"); - assert!(file.matches_file("dir/dir")); + assert!(file.matches("dir/dir")); // We don't want the "dir" pattern to apply to the parent directory - assert!(!file.matches_file("dir/foo")); + assert!(!file.matches("dir/foo")); } #[test] fn test_gitignore_rooted_literal() { let file = GitIgnoreFile::empty().chain("", b"/foo\n"); - assert!(file.matches_file("foo")); - assert!(!file.matches_file("dir/foo")); + assert!(file.matches("foo")); + assert!(!file.matches("dir/foo")); } #[test] @@ -296,98 +279,98 @@ mod tests { let file = GitIgnoreFile::empty().chain("dir/", b"/foo\n"); // I consider it undefined whether a file in a parent directory matches, but // let's test it anyway - assert!(!file.matches_file("foo")); - assert!(file.matches_file("dir/foo")); - assert!(!file.matches_file("dir/subdir/foo")); + assert!(!file.matches("foo")); + 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"); - assert!(!file.matches_file("foo")); - assert!(!file.matches_file("dir1/foo")); - assert!(!file.matches_file("dir1/dir2/foo")); - assert!(file.matches_file("dir1/dir2/dir3/foo")); - assert!(file.matches_file("dir1/dir2/dir3/dir4/foo")); + assert!(!file.matches("foo")); + assert!(!file.matches("dir1/foo")); + assert!(!file.matches("dir1/dir2/foo")); + assert!(file.matches("dir1/dir2/dir3/foo")); + assert!(file.matches("dir1/dir2/dir3/dir4/foo")); } #[test] fn test_gitignore_match_only_dir() { let file = GitIgnoreFile::empty().chain("", b"/dir/\n"); - assert!(!file.matches_file("dir")); - assert!(file.matches_file("dir/foo")); - assert!(file.matches_file("dir/subdir/foo")); + assert!(!file.matches("dir")); + assert!(file.matches("dir/foo")); + assert!(file.matches("dir/subdir/foo")); } #[test] fn test_gitignore_unusual_symbols() { - assert!(matches_file(b"\\*\n", "*")); - assert!(!matches_file(b"\\*\n", "foo")); - assert!(matches_file(b"\\\n", "\\")); - assert!(matches_file(b"\\!\n", "!")); - assert!(matches_file(b"\\?\n", "?")); - assert!(!matches_file(b"\\?\n", "x")); + assert!(matches(b"\\*\n", "*")); + assert!(!matches(b"\\*\n", "foo")); + assert!(matches(b"\\\n", "\\")); + assert!(matches(b"\\!\n", "!")); + assert!(matches(b"\\?\n", "?")); + assert!(!matches(b"\\?\n", "x")); // Invalid escapes are treated like literal backslashes - assert!(matches_file(b"\\w\n", "\\w")); - assert!(!matches_file(b"\\w\n", "w")); + assert!(matches(b"\\w\n", "\\w")); + assert!(!matches(b"\\w\n", "w")); } #[test] fn test_gitignore_whitespace() { - assert!(!matches_file(b" \n", " ")); - assert!(matches_file(b"\\ \n", " ")); - assert!(matches_file(b"\\\\ \n", "\\")); - assert!(!matches_file(b"\\\\ \n", " ")); - assert!(matches_file(b"\\\\\\ \n", "\\ ")); - assert!(matches_file(b" a\n", " a")); - assert!(matches_file(b"a b\n", "a b")); - assert!(matches_file(b"a b \n", "a b")); - assert!(!matches_file(b"a b \n", "a b ")); - assert!(matches_file(b"a b\\ \\ \n", "a b ")); + assert!(!matches(b" \n", " ")); + assert!(matches(b"\\ \n", " ")); + assert!(matches(b"\\\\ \n", "\\")); + assert!(!matches(b"\\\\ \n", " ")); + assert!(matches(b"\\\\\\ \n", "\\ ")); + assert!(matches(b" a\n", " a")); + assert!(matches(b"a b\n", "a b")); + assert!(matches(b"a b \n", "a b")); + assert!(!matches(b"a b \n", "a b ")); + assert!(matches(b"a b\\ \\ \n", "a b ")); // It's unclear how this should be interpreted, but we count spaces before // escaped spaces - assert!(matches_file(b"a b \\ \n", "a b ")); + assert!(matches(b"a b \\ \n", "a b ")); // A single CR at EOL is ignored - assert!(matches_file(b"a\r\n", "a")); - assert!(!matches_file(b"a\r\n", "a\r")); - assert!(matches_file(b"a\r\r\n", "a\r")); - assert!(!matches_file(b"a\r\r\n", "a\r\r")); - assert!(matches_file(b"\ra\n", "\ra")); - assert!(!matches_file(b"\ra\n", "a")); + assert!(matches(b"a\r\n", "a")); + assert!(!matches(b"a\r\n", "a\r")); + assert!(matches(b"a\r\r\n", "a\r")); + assert!(!matches(b"a\r\r\n", "a\r\r")); + assert!(matches(b"\ra\n", "\ra")); + assert!(!matches(b"\ra\n", "a")); } #[test] fn test_gitignore_glob() { - assert!(!matches_file(b"*.o\n", "foo")); - assert!(matches_file(b"*.o\n", "foo.o")); - assert!(!matches_file(b"foo.?\n", "foo")); - assert!(!matches_file(b"foo.?\n", "foo.")); - assert!(matches_file(b"foo.?\n", "foo.o")); + assert!(!matches(b"*.o\n", "foo")); + assert!(matches(b"*.o\n", "foo.o")); + assert!(!matches(b"foo.?\n", "foo")); + assert!(!matches(b"foo.?\n", "foo.")); + assert!(matches(b"foo.?\n", "foo.o")); } #[test] fn test_gitignore_range() { - assert!(!matches_file(b"foo.[az]\n", "foo")); - assert!(matches_file(b"foo.[az]\n", "foo.a")); - assert!(!matches_file(b"foo.[az]\n", "foo.g")); - assert!(matches_file(b"foo.[az]\n", "foo.z")); - assert!(!matches_file(b"foo.[a-z]\n", "foo")); - assert!(matches_file(b"foo.[a-z]\n", "foo.a")); - assert!(matches_file(b"foo.[a-z]\n", "foo.g")); - assert!(matches_file(b"foo.[a-z]\n", "foo.z")); - assert!(matches_file(b"foo.[0-9a-fA-F]\n", "foo.5")); - assert!(matches_file(b"foo.[0-9a-fA-F]\n", "foo.c")); - assert!(matches_file(b"foo.[0-9a-fA-F]\n", "foo.E")); - assert!(!matches_file(b"foo.[0-9a-fA-F]\n", "foo._")); + assert!(!matches(b"foo.[az]\n", "foo")); + assert!(matches(b"foo.[az]\n", "foo.a")); + assert!(!matches(b"foo.[az]\n", "foo.g")); + assert!(matches(b"foo.[az]\n", "foo.z")); + assert!(!matches(b"foo.[a-z]\n", "foo")); + assert!(matches(b"foo.[a-z]\n", "foo.a")); + assert!(matches(b"foo.[a-z]\n", "foo.g")); + assert!(matches(b"foo.[a-z]\n", "foo.z")); + assert!(matches(b"foo.[0-9a-fA-F]\n", "foo.5")); + assert!(matches(b"foo.[0-9a-fA-F]\n", "foo.c")); + assert!(matches(b"foo.[0-9a-fA-F]\n", "foo.E")); + assert!(!matches(b"foo.[0-9a-fA-F]\n", "foo._")); } #[test] fn test_gitignore_leading_dir_glob() { - assert!(matches_file(b"**/foo\n", "foo")); - assert!(matches_file(b"**/foo\n", "dir1/dir2/foo")); - assert!(matches_file(b"**/foo\n", "foo/file")); - assert!(matches_file(b"**/dir/foo\n", "dir/foo")); - assert!(matches_file(b"**/dir/foo\n", "dir1/dir2/dir/foo")); + assert!(matches(b"**/foo\n", "foo")); + assert!(matches(b"**/foo\n", "dir1/dir2/foo")); + assert!(matches(b"**/foo\n", "foo/file")); + assert!(matches(b"**/dir/foo\n", "dir/foo")); + assert!(matches(b"**/dir/foo\n", "dir1/dir2/dir/foo")); } #[test] @@ -395,46 +378,46 @@ mod tests { let file = GitIgnoreFile::empty().chain("dir1/dir2/", b"**/foo\n"); // I consider it undefined whether a file in a parent directory matches, but // let's test it anyway - assert!(!file.matches_file("foo")); - assert!(file.matches_file("dir1/dir2/foo")); - assert!(!file.matches_file("dir1/dir2/bar")); - assert!(file.matches_file("dir1/dir2/sub1/sub2/foo")); - assert!(!file.matches_file("dir1/dir2/sub1/sub2/bar")); + assert!(!file.matches("foo")); + assert!(file.matches("dir1/dir2/foo")); + assert!(!file.matches("dir1/dir2/bar")); + assert!(file.matches("dir1/dir2/sub1/sub2/foo")); + assert!(!file.matches("dir1/dir2/sub1/sub2/bar")); } #[test] fn test_gitignore_trailing_dir_glob() { - assert!(!matches_file(b"abc/**\n", "abc")); - assert!(matches_file(b"abc/**\n", "abc/file")); - assert!(matches_file(b"abc/**\n", "abc/dir/file")); + assert!(!matches(b"abc/**\n", "abc")); + assert!(matches(b"abc/**\n", "abc/file")); + assert!(matches(b"abc/**\n", "abc/dir/file")); } #[test] fn test_gitignore_internal_dir_glob() { - assert!(matches_file(b"a/**/b\n", "a/b")); - assert!(matches_file(b"a/**/b\n", "a/x/b")); - assert!(matches_file(b"a/**/b\n", "a/x/y/b")); - assert!(!matches_file(b"a/**/b\n", "ax/y/b")); - assert!(!matches_file(b"a/**/b\n", "a/x/yb")); - assert!(!matches_file(b"a/**/b\n", "ab")); + assert!(matches(b"a/**/b\n", "a/b")); + assert!(matches(b"a/**/b\n", "a/x/b")); + assert!(matches(b"a/**/b\n", "a/x/y/b")); + assert!(!matches(b"a/**/b\n", "ax/y/b")); + assert!(!matches(b"a/**/b\n", "a/x/yb")); + assert!(!matches(b"a/**/b\n", "ab")); } #[test] fn test_gitignore_internal_dir_glob_not_really() { - assert!(!matches_file(b"a/x**y/b\n", "a/b")); - assert!(matches_file(b"a/x**y/b\n", "a/xy/b")); - assert!(matches_file(b"a/x**y/b\n", "a/xzzzy/b")); + assert!(!matches(b"a/x**y/b\n", "a/b")); + assert!(matches(b"a/x**y/b\n", "a/xy/b")); + assert!(matches(b"a/x**y/b\n", "a/xzzzy/b")); } #[test] fn test_gitignore_line_ordering() { - assert!(matches_file(b"foo\n!foo/bar\n", "foo")); - assert!(!matches_file(b"foo\n!foo/bar\n", "foo/bar")); - assert!(matches_file(b"foo\n!foo/bar\n", "foo/baz")); - assert!(matches_file(b"foo\n!foo/bar\nfoo/bar/baz", "foo")); - assert!(!matches_file(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar")); - assert!(matches_file(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar/baz")); - assert!(!matches_file(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar/quux")); + assert!(matches(b"foo\n!foo/bar\n", "foo")); + assert!(!matches(b"foo\n!foo/bar\n", "foo/bar")); + assert!(matches(b"foo\n!foo/bar\n", "foo/baz")); + assert!(matches(b"foo\n!foo/bar\nfoo/bar/baz", "foo")); + assert!(!matches(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar")); + assert!(matches(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar/baz")); + assert!(!matches(b"foo\n!foo/bar\nfoo/bar/baz", "foo/bar/quux")); } #[test] @@ -442,21 +425,11 @@ mod tests { let file1 = GitIgnoreFile::empty().chain("", b"foo\n"); let file2 = file1.chain("foo/", b"!bar"); let file3 = file2.chain("foo/bar/", b"baz"); - assert!(file1.matches_file("foo")); - assert!(file1.matches_file("foo/bar")); - assert!(!file2.matches_file("foo/bar")); - assert!(file2.matches_file("foo/baz")); - assert!(file3.matches_file("foo/bar/baz")); - assert!(!file3.matches_file("foo/bar/qux")); - } - - #[test] - fn test_gitignore_match_dir() { - assert!(matches_all_files_in(b"foo\n", "foo/")); - assert!(matches_all_files_in(b"foo\nbar\n", "foo/")); - assert!(matches_all_files_in(b"!foo\nbar\n", "bar/")); - assert!(!matches_all_files_in(b"foo\n!bar\n", "foo/")); - // This one could return true, but it doesn't currently - assert!(!matches_all_files_in(b"foo\n!/bar\n", "foo/")); + assert!(file1.matches("foo")); + assert!(file1.matches("foo/bar")); + assert!(!file2.matches("foo/bar")); + assert!(file2.matches("foo/baz")); + assert!(file3.matches("foo/bar/baz")); + assert!(!file3.matches("foo/bar/qux")); } } diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index aae9b3fe5..6eaec5be3 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -796,7 +796,7 @@ impl TreeState { } if file_type.is_dir() { - if git_ignore.matches_all_files_in(&path.to_internal_dir_string()) { + if git_ignore.matches(&path.to_internal_dir_string()) { // If the whole directory is ignored, visit only paths we're already // tracking. let tracked_paths = self @@ -867,7 +867,7 @@ impl TreeState { } let maybe_current_file_state = self.file_states.get(&path); if maybe_current_file_state.is_none() - && git_ignore.matches_file(&path.to_internal_file_string()) + && git_ignore.matches(&path.to_internal_file_string()) { // If it wasn't already tracked and it matches // the ignored paths, then