Adds .gitignore support #429

osklyar · 2017-06-15T15:40:01Z

This is a follow up from #420. As I am not sure if the PR approach is the one that you expect, I request feedback on this WIP change before I introduce all the tests, which are the only missing bits.

Improvements on top of the original PR #420:

As requested, the functionality is added into Worktree this time.
Because the initially used library for matching gitignore patterns had a lot of gaps concerning even some simple cases I have written a new one from scratch, fully tested to the gitignore pattern specification. I will round up the documentation for that one in due course, but it is both feature and test complete.
Further, this PR introduces support for nested .gitignore files according to the specification, not just the top level one as in Adds support for .gitignore at repository root #420. It would be easily extensible to higher prio patterns given on a command line or lower prio patterns from the environment.

mcuadros · 2017-06-15T22:03:58Z

@osklyar we will be happy to accept also the gitignore parser as a contribution.

osklyar · 2017-06-15T22:11:32Z

@mcuadros Cool. As src-d/go-gitignore or src-d/go-git/gitignore? If the former please prepare a repo so I could submit a PR. I will then submit the PR and will change the paths correspondingly. Before that I will move the directory walker code for .gitignore file discovery from Worktree into gitignore, add docs and README

codecov · 2017-06-16T07:28:55Z

Codecov Report

Merging #429 into master will decrease coverage by 0.35%.
The diff coverage is 93.38%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
- Coverage   77.77%   77.41%   -0.36%     
==========================================
  Files         124      127       +3     
  Lines        9027     9140     +113     
==========================================
+ Hits         7021     7076      +55     
- Misses       1231     1303      +72     
+ Partials      775      761      -14

Impacted Files	Coverage Δ
plumbing/format/gitignore/pattern.go	`100% <100%> (ø)`
plumbing/format/gitignore/dir.go	`80% <80%> (ø)`
worktree_status.go	`71.21% <85%> (+1.54%)`	⬆️
plumbing/format/gitignore/matcher.go	`87.5% <87.5%> (ø)`
plumbing/transport/ssh/common.go	`0% <0%> (-50.91%)`	⬇️
plumbing/transport/ssh/auth_method.go	`33.33% <0%> (-24.77%)`	⬇️
repository.go	`72.87% <0%> (-0.3%)`	⬇️
worktree.go	`64.52% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d1c674...2f4ac21. Read the comment docs.

mcuadros · 2017-06-16T07:29:55Z

@osklyar as part of go-git, we have a bunch of parser already so make sense of having all together at:
https://github.com/src-d/go-git/tree/master/plumbing/format

osklyar · 2017-06-16T07:35:02Z

@mcuadros It is a fully independent gitignore parser and judging from stars on the one I originally used there is some interest in using such a parser elsewhere. Sticking it deep into plumbing would make it cumbersome where you need to pull a complete go-git dependency just to use the matcher.

I am happy to put it where you suggest but then I would keep the original repo. If, however, this remains an independent repo, I would be happy to transfer the ownership to src-d. Let me know.

The gitignore code is now complete with tests and documentation, please check if it is ok like this.

mcuadros · 2017-06-16T07:44:26Z

Having one of the parser as dependency may be ending problematic, if a fix is required. I don't see a problem having nested packaged, if they are well document they appear in godoc searches as any other package.

github.com/monochromegane/go-gitignore has 17 stars

osklyar · 2017-06-16T13:07:01Z

Now ready, taking off the WIP label:

the gitignore matcher commited into plumbing/format/gitignore
it is made to use billy.FileSystem instead of the original wrapper
in addition to all original tests from gitignore added a test for walking the directory structure reading in .gitignore files via billy.FileSystem
added a test for gitignore support in Worktree.Status

osklyar · 2017-06-16T13:22:38Z

plumbing/format/gitignore/dir_test.go

+	"gopkg.in/src-d/go-billy.v2"
+	"gopkg.in/src-d/go-billy.v2/memfs"
+	"gopkg.in/src-d/go-git.v4/plumbing/format/gitignore"
+	"github.com/silvertern/geat/client"


oops. fixing dependency

mcuadros

Awesome contribution! Thanks this is something that was really missing and very important.

mcuadros · 2017-06-17T10:40:12Z

plumbing/format/gitignore/dir_test.go

@@ -0,0 +1,42 @@
+package gitignore_test


we don't use dedicated test packages, can you use just gitignore as package?

mcuadros · 2017-06-17T10:41:37Z

plumbing/format/gitignore/README.md

@@ -0,0 +1,71 @@
+# `gitignore` parser and matcher for Go


Instead of a README.md we document the packages as package documentation as:
https://github.com/src-d/go-git/blob/master/plumbing/format/index/doc.go

And this appears at godoc like: https://godoc.org/gopkg.in/src-d/go-git.v4/plumbing/format/index

mcuadros · 2017-06-17T10:42:52Z

plumbing/format/gitignore/dir.go

+
+import (
+	"strings"
+	"gopkg.in/src-d/go-billy.v2"


separate the import groups

mcuadros · 2017-06-17T10:43:20Z

plumbing/format/gitignore/dir.go

+
+// ReadPatterns reads gitignore patterns recursively traversing through the directory
+// structure. The result is in the ascending order of priority (last higher).
+func ReadPatterns(fs billy.Filesystem, path []string) (patterns []Pattern, err error) {


ReadPatterns(fs billy.Filesystem, path []string) (p []Pattern, err error)

Will do (ps []Pattern, err error)

mcuadros · 2017-06-17T10:43:57Z

plumbing/format/gitignore/dir.go

+		defer f.Close()
+		if data, err := ioutil.ReadAll(f); err == nil {
+			for _, s := range strings.Split(string(data), "\n") {
+				if !strings.HasPrefix(s, "#") && len(strings.TrimSpace(s)) > 0 {


can you put # in a constant to give it semantic meaning?

mcuadros · 2017-06-17T10:45:00Z

plumbing/format/gitignore/matcher.go

@@ -0,0 +1,33 @@
+// gitignore package implements scanning for and parsing of gitignore files in a directory


move this comment to a doc.go file, dedicated to the package documentation including the content of the README.md

mcuadros · 2017-06-17T10:45:57Z

plumbing/format/gitignore/pattern_test.go

@@ -0,0 +1,321 @@
+package gitignore_test


gitignore

mcuadros · 2017-06-17T10:46:02Z

plumbing/format/gitignore/pattern_test.go

+package gitignore_test
+
+import (
+	"gopkg.in/src-d/go-git.v4/plumbing/format/gitignore"


mcuadros · 2017-06-18T10:00:44Z

Can you squash the commit in a meaningful way? The last thing to merge this. :D

osklyar · 2017-06-18T22:30:52Z

@mcuadros Now squashed into one commit. The process required resetting all the changed on the branch at which point the PR was automatically closed, so I reopened it now with that single commit.

mcuadros · 2017-06-18T22:41:07Z

plumbing/format/gitignore/dir.go

+	"io/ioutil"
+	"strings"
+
+	"gopkg.in/src-d/go-billy.v2"


test are failing, the master now use go-billy.v3

As you merged the request, I understand you have already updated this? Is there anything left for me to do?

osklyar changed the title ~~WIP: Adds .gitignore support~~ Adds .gitignore support Jun 16, 2017

osklyar commented Jun 16, 2017

View reviewed changes

osklyar mentioned this pull request Jun 16, 2017

Walking through tree of non gitignored files #307

Closed

mcuadros suggested changes Jun 17, 2017

View reviewed changes

mcuadros requested a review from smola June 17, 2017 10:47

mcuadros approved these changes Jun 18, 2017

View reviewed changes

osklyar closed this Jun 18, 2017

Adds gitignore support

2f4ac21

osklyar reopened this Jun 18, 2017

mcuadros suggested changes Jun 18, 2017

View reviewed changes

mcuadros merged commit 93633b5 into src-d:master Jun 19, 2017

osklyar mentioned this pull request Jun 20, 2017

Fixes checkout not possible with (untracked) files under gitignore #444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds .gitignore support #429

Adds .gitignore support #429

osklyar commented Jun 15, 2017

mcuadros commented Jun 15, 2017

osklyar commented Jun 15, 2017

codecov bot commented Jun 16, 2017 •

edited

Loading

mcuadros commented Jun 16, 2017

osklyar commented Jun 16, 2017

mcuadros commented Jun 16, 2017 •

edited

Loading

osklyar commented Jun 16, 2017

osklyar Jun 16, 2017

mcuadros left a comment

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

osklyar Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros Jun 17, 2017

mcuadros commented Jun 18, 2017

osklyar commented Jun 18, 2017

mcuadros Jun 18, 2017

osklyar Jun 19, 2017

		@@ -0,0 +1,33 @@
		// gitignore package implements scanning for and parsing of gitignore files in a directory

Adds .gitignore support #429

Adds .gitignore support #429

Conversation

osklyar commented Jun 15, 2017

mcuadros commented Jun 15, 2017

osklyar commented Jun 15, 2017

codecov bot commented Jun 16, 2017 • edited Loading

Codecov Report

mcuadros commented Jun 16, 2017

osklyar commented Jun 16, 2017

mcuadros commented Jun 16, 2017 • edited Loading

osklyar commented Jun 16, 2017

Choose a reason for hiding this comment

mcuadros 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

Choose a reason for hiding this comment

mcuadros commented Jun 18, 2017

osklyar commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2017 •

edited

Loading

mcuadros commented Jun 16, 2017 •

edited

Loading