Add basic ignore functionality #71

sirnewton01 · 2018-07-23T02:23:05Z

Adds a new check-ignore command. The status command uses the check-ignore backing logic to filter out any untracked changes. The official git t0008-ignores.sh suite went from completely failing to 297/392 passing.

… t0008

driusan · 2018-07-23T13:05:06Z

git/status.go

@@ -346,6 +346,11 @@ func StatusLong(c *Client, files []File, untracked StatusUntrackedMode, linepref
 				if err != nil {
 					return "", err
 				}
+				// TODO instead of filtering afterwards, perhaps the untracked should be filtered at the source
+				matches, _ := CheckIgnore(c, CheckIgnoreOptions{}, []File{fname})


Why don't we put this check in LsFiles and include ExcludeStandard in the opts? That would go a lot farther than adding a filter in every porcelain command

sirnewton01 · 2018-07-26T01:55:21Z

The latest commits add more capabilities with the ignore patterns. Next step is to integrate the ignores into ls-files so that other plumbing commands get the benefit, such as status and add.

…nds can use it via ls-files

sirnewton01 · 2018-07-26T02:17:20Z

Thanks @driusan I see that you anticipated that the LsFiles() function would be the place where ignore capabilities are centralized. Hooking it into status/add was really straight forward. With the latest changes in this PR the t0008-ignores.sh official test has only 87 failures out of 392. I'm going to try to whittle this number down further so that I can add it to the travis CI.

sirnewton01 · 2018-07-26T04:15:31Z

official-git/fix-ignores-test.patch

+ 		git init &&
+ 		echo a >a &&
+ 		git add a &&
+-		git commit -m"commit in submodule"


There's no space between the -m and the commit message, which causes go's flags package to fail on it and also the git manual specifies a space between -m and the message. It seems that the official git command-line just supports this unspecified syntax and the test here happens to use it.

driusan

Some suggestions for cleanup, but I don't mind merging this as-is if you prefer, because it's still a huge step forward. Just let me know when you're done enough that you want it merged.

driusan · 2018-07-26T11:33:05Z

git/checkignore.go

+
+	for idx, path := range paths {
+		if !opts.NoIndex {
+			log.Printf("Checking if %s is tracked by git\n", path.String())


I think the stringer call is implicit since your formatting directive is "%s"

driusan · 2018-07-26T11:40:19Z

git/checkignore.go

+		// Be sure to assign the pathname in all cases so that clients can match the outputs to their inputs
+		patternMatches[idx].PathName = path
+
+		// TODO: consider the other places where ignores can come from, such as core.excludesFile and .git/info/exclude


I think the way to do this is to invert your logic: check-ignore should call ls-files, ls-files shouldn't call check-ignore. ls-files has the exclude-per-directory and exclude-from options, while check-ignore doesn't have any way to customize

I was thinking about this also. The trouble in my thinking comes with the check-ignore --no-index option. I'm not sure how to push that option down into ls-files.

I'm not an expert on the exclude semantics, but from how I interpret the man page it just affects whether it calls ls-files -o or ls-files -c -o

driusan · 2018-07-26T11:45:12Z

git/checkignore.go

+	return "", 0, nil
+}
+
+func matchesGlob(path string, isDir bool, pattern string) bool {


Why not replace the File.MatchGlob I added with this instead of adding a new way to do it?

For now, I was going to keep this local because it doesn't quite fit the File.MatchGlob pattern after all. For example, this ones needs to know if the path points to a directory or not for some of the matching logic, but the path may be relative to arbitrary positions within the work directory.

driusan · 2018-07-26T12:03:38Z

git/checkignore.go

+			gitignore := filepath.Join(dir, ".gitignore")
+			log.Printf("Checking .gitignore in %s\n", gitignore)
+			ignorePath, err := filepath.Rel(dir, abs)
+			pattern, lineNumber, err := findPatternInIgnoreFile(c, gitignore, ignorePath, isDir)


What about matches that come from .gitignores higher up in the tree? I think the logic for directories is easier to reason about recursively than iteratively. For instance, we could parse the ignore file and return a slice of patterns, then pass that slice to children to deal with parent ignores. (Which also has the advantages of only parsing the file once). If that was done and the ls-files inversion, it would be easy to add exclude-per-directory and exclude-from

Yes, I could think about this a bit more. Some of the complexity comes from the fact that in order to match a pattern in a gitignore file you need to make the path of the file to be ignore/not ignored relative to that gitignore file in order to check the matches. That makes it a bit more difficult to use patterns from a git ignore file out of context, but perhaps the gitignore parse result could be made into a type that has helper methods on it to prevent mistakes in usage.

driusan

Drive-by review.

(It's really impressive how close this is getting to passing all the tests considering how many tests there are. )

driusan · 2018-07-29T00:28:49Z

git/ignore_test.go

+	}
+}
+
+func TestIgnorePatternMatches(t *testing.T) {


This test seems to want to be a table driven test

driusan · 2018-07-29T00:34:37Z

git/ignore.go

+	defer file.Close()
+
+	ignorePatterns := []IgnorePattern{}
+	reader := bufio.NewReader(file)


I haven't looked into this loop in detail, but from a cursory glance it seems that this might be easier using a bufio.Scanner to scan lines instead of a bufio.Reader

I wish that were the case, but unfortunately buffo.Scanner will miss the last line of a stream if there is no newline character at the end. :( This is why there is the pain of having to use the reader directly.

driusan · 2018-07-29T00:38:21Z

git/ignore.go

+			return nil, err
+		}
+
+		// Let's check that this path is in the git work dir first


This check seems useful enough that it might make sense to make it a method on File rather than inline in the loop.

driusan · 2018-07-29T00:41:30Z

git/file.go

@@ -72,6 +72,45 @@ func (f File) IsDir() bool {

 }

+// Returns true if the file is inside a submodule and the submodule file.
+func (f File) IsInSubmodule(c *Client) (bool, File, error) {


Can you add a FIXME saying that when we implement submodules this should be inverted? (Submodules contain Files, not the other way around, but we don't currently have a submodule type..)

driusan · 2018-07-29T00:45:28Z

cmd/checkignore.go

+
+		// Invoke the ignore routines directly, rather than relying on
+		//  LsFiles because we need to be able to do things such as
+		//  return non-matches and match details that aren't supported there.


I don't think I understand this comment. Isn't determining whether nonmatches or matches are returned what the -i option (which admittedly isn't currently implemented, but that's effectively what this PR is doing) toggles? What details that aren't supported need to be matched?

Right the "-i" option is not yet enabled and the LsFiles won't return back the pattern that matched or wasn't matched so that checkignore can output that with the -v flag.

sirnewton01 · 2018-07-29T00:59:54Z

@driusan I've reorganized things here quite a bit hoping that it is a much cleaner implementation with better separation of concerns. I've gotten the number of official git tests down to only 28 out of 395 for the ignore tests. The remainder are mostly related to the fact that I have not yet implemented the negation '!' operator and a couple of edge cases with escaping of whitespace vs. the slash character itself.

…ation or escaping edge cases

sirnewton01 · 2018-07-29T02:15:55Z

I have added all of the passing official git ignore tests to travis to catch regressions. The majority of the failing tests are because of the negation operator, which I think should be tackled as a separate issue.

sirnewton01 added 13 commits July 22, 2018 10:41

Add basic check ignore command structure

ac1d658

Add quiet flag

ffb9aaf

Add verbose flag

e222fdf

Fix glob pattern matching for sub-directories

c049d4c

Hook check ignores into the status command

0a458ad

Add non-matching capability and adjust to suit official test cases in…

3e99339

… t0008

Add stdin support and make more official tests pass

02a8d9f

Add no-index option

a1a7fd0

Add -z support

2cce885

Check if running check-ignore from the gitdir

b18fabd

Add support for getting ignores from .git/info/excludes file

ecf1303

Fix build

23d4720

Update check-ignore usage

8ccb935

sirnewton01 mentioned this pull request Jul 23, 2018

Support for check-ignore #26

Closed

driusan reviewed Jul 23, 2018

View reviewed changes

sirnewton01 added 2 commits July 25, 2018 20:40

Support for more complex glob patterns

0ff6b23

Make the glob matching private

e530eee

Pull the check-ignore functionality into ls-files so that other comma…

b9cba91

…nds can use it via ls-files

sirnewton01 added 4 commits July 25, 2018 23:13

Fix more test cases, add symbolic link pass through detection

90b7bcf

Run ignores test, but ignore failure

98d8b4f

Detect check-ignore on submodule paths

7bab0b3

Add ignores tests patch

78fadc5

sirnewton01 commented Jul 26, 2018

View reviewed changes

driusan requested changes Jul 26, 2018

View reviewed changes

sirnewton01 added 4 commits July 26, 2018 08:56

Code review feedback

1a4cf1f

Re-arrange ignore handling to satisfy check-ignore and ls-files

22ce50a

Fix the 'corner case'

decb680

Fix some of the lsfiles tests

50c1ba9

Fix sort function for ignore precendence

103789b

driusan reviewed Jul 29, 2018

View reviewed changes

sirnewton01 added 3 commits July 28, 2018 21:04

Add core.excludesfile to the standard ignore patterns if it is present

34086f3

Add fixme for the IsInSubmodule function for File

eb933e3

Add the ignores test suite ignoring the cases that are related to neg…

3beb427

…ation or escaping edge cases

driusan merged commit dafef00 into driusan:master Jul 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic ignore functionality #71

Add basic ignore functionality #71

sirnewton01 commented Jul 23, 2018

driusan Jul 23, 2018 •

edited

Loading

sirnewton01 commented Jul 26, 2018

sirnewton01 commented Jul 26, 2018

sirnewton01 Jul 26, 2018

driusan left a comment

driusan Jul 26, 2018

driusan Jul 26, 2018

sirnewton01 Jul 26, 2018

driusan Jul 26, 2018

driusan Jul 26, 2018

sirnewton01 Jul 26, 2018

driusan Jul 26, 2018

sirnewton01 Jul 26, 2018 •

edited

Loading

driusan left a comment

driusan Jul 29, 2018

driusan Jul 29, 2018

sirnewton01 Jul 29, 2018

driusan Jul 29, 2018

driusan Jul 29, 2018

sirnewton01 Jul 29, 2018

driusan Jul 29, 2018

sirnewton01 Jul 29, 2018

sirnewton01 commented Jul 29, 2018

sirnewton01 commented Jul 29, 2018

Add basic ignore functionality #71

Add basic ignore functionality #71

Conversation

sirnewton01 commented Jul 23, 2018

driusan Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

sirnewton01 commented Jul 26, 2018

sirnewton01 commented Jul 26, 2018

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirnewton01 Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirnewton01 commented Jul 29, 2018

sirnewton01 commented Jul 29, 2018

driusan Jul 23, 2018 •

edited

Loading

sirnewton01 Jul 26, 2018 •

edited

Loading