lib/ignore: Keep skipping ignored dirs for rooted patterns (#6151)

* lib/ignore: Keep skipping ignored dirs for rooted patterns

* review

* clarify comment and lint

* glob.QuoteMeta

* review

lib/ignore/ignore.go

@@ -32,6 +32,14 @@ const (
 	resultFoldCase          = 1 << iota
 )
 
+var defaultResult Result = resultInclude
+
+func init() {
+	if runtime.GOOS == "darwin" || runtime.GOOS == "windows" {
+		defaultResult |= resultFoldCase
+	}
+}
+
 type Pattern struct {
 	pattern string
 	match   glob.Glob
@@ -52,6 +60,25 @@ func (p Pattern) String() string {
 	return ret
 }
 
+func (p Pattern) allowsSkippingIgnoredDirs() bool {
+	if p.result.IsIgnored() {
+		return true
+	}
+	if p.pattern[0] != '/' {
+		return false
+	}
+	// Double asterisk everywhere in the path except at the end is bad
+	if strings.Contains(strings.TrimSuffix(p.pattern, "**"), "**") {
+		return false
+	}
+	// Any wildcards anywhere except for the last path component are bad
+	lastSep := strings.LastIndex(p.pattern, "/")
+	if lastSep == -1 {
+		return true
+	}
+	return p.pattern[:lastSep] == glob.QuoteMeta(p.pattern[:lastSep])
+}
+
 type Result uint8
 
 func (r Result) IsIgnored() bool {
@@ -174,11 +201,20 @@ func (m *Matcher) parseLocked(r io.Reader, file string) error {
 	}
 
 	m.skipIgnoredDirs = true
+	var previous string
 	for _, p := range patterns {
-		if !p.result.IsIgnored() {
+		// We automatically add patterns with a /** suffix, which normally
+		// means that we cannot skip directories. However if the same
+		// pattern without the /** already exists (which is true for
+		// automatically added patterns) we can skip.
+		if l := len(p.pattern); l > 3 && p.pattern[:len(p.pattern)-3] == previous {
+			continue
+		}
+		if !p.allowsSkippingIgnoredDirs() {
 			m.skipIgnoredDirs = false
 			break
 		}
+		previous = p.pattern
 	}
 
 	m.curHash = newHash
@@ -360,87 +396,90 @@ func loadParseIncludeFile(filesystem fs.Filesystem, file string, cd ChangeDetect
 	return patterns, err
 }
 
+func parseLine(line string) ([]Pattern, error) {
+	pattern := Pattern{
+		result: defaultResult,
+	}
+
+	// Allow prefixes to be specified in any order, but only once.
+	var seenPrefix [3]bool
+
+	for {
+		if strings.HasPrefix(line, "!") && !seenPrefix[0] {
+			seenPrefix[0] = true
+			line = line[1:]
+			pattern.result ^= resultInclude
+		} else if strings.HasPrefix(line, "(?i)") && !seenPrefix[1] {
+			seenPrefix[1] = true
+			pattern.result |= resultFoldCase
+			line = line[4:]
+		} else if strings.HasPrefix(line, "(?d)") && !seenPrefix[2] {
+			seenPrefix[2] = true
+			pattern.result |= resultDeletable
+			line = line[4:]
+		} else {
+			break
+		}
+	}
+
+	if pattern.result.IsCaseFolded() {
+		line = strings.ToLower(line)
+	}
+
+	pattern.pattern = line
+
+	var err error
+	if strings.HasPrefix(line, "/") {
+		// Pattern is rooted in the current dir only
+		pattern.match, err = glob.Compile(line[1:], '/')
+		return []Pattern{pattern}, err
+	}
+	patterns := make([]Pattern, 2)
+	if strings.HasPrefix(line, "**/") {
+		// Add the pattern as is, and without **/ so it matches in current dir
+		pattern.match, err = glob.Compile(line, '/')
+		if err != nil {
+			return nil, err
+		}
+		patterns[0] = pattern
+
+		line = line[3:]
+		pattern.pattern = line
+		pattern.match, err = glob.Compile(line, '/')
+		if err != nil {
+			return nil, err
+		}
+		patterns[1] = pattern
+		return patterns, nil
+	}
+	// Path name or pattern, add it so it matches files both in
+	// current directory and subdirs.
+	pattern.match, err = glob.Compile(line, '/')
+	if err != nil {
+		return nil, err
+	}
+	patterns[0] = pattern
+
+	line = "**/" + line
+	pattern.pattern = line
+	pattern.match, err = glob.Compile(line, '/')
+	if err != nil {
+		return nil, err
+	}
+	patterns[1] = pattern
+	return patterns, nil
+}
+
 func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd ChangeDetector, linesSeen map[string]struct{}) ([]string, []Pattern, error) {
 	var lines []string
 	var patterns []Pattern
 
-	defaultResult := resultInclude
-	if runtime.GOOS == "darwin" || runtime.GOOS == "windows" {
-		defaultResult |= resultFoldCase
-	}
-
 	addPattern := func(line string) error {
-		pattern := Pattern{
+		newPatterns, err := parseLine(line)
-			result: defaultResult,
+		if err != nil {
-		}
+			return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-
-		// Allow prefixes to be specified in any order, but only once.
-		var seenPrefix [3]bool
-
-		for {
-			if strings.HasPrefix(line, "!") && !seenPrefix[0] {
-				seenPrefix[0] = true
-				line = line[1:]
-				pattern.result ^= resultInclude
-			} else if strings.HasPrefix(line, "(?i)") && !seenPrefix[1] {
-				seenPrefix[1] = true
-				pattern.result |= resultFoldCase
-				line = line[4:]
-			} else if strings.HasPrefix(line, "(?d)") && !seenPrefix[2] {
-				seenPrefix[2] = true
-				pattern.result |= resultDeletable
-				line = line[4:]
-			} else {
-				break
-			}
-		}
-
-		if pattern.result.IsCaseFolded() {
-			line = strings.ToLower(line)
-		}
-
-		pattern.pattern = line
-
-		var err error
-		if strings.HasPrefix(line, "/") {
-			// Pattern is rooted in the current dir only
-			pattern.match, err = glob.Compile(line[1:], '/')
-			if err != nil {
-				return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-			}
-			patterns = append(patterns, pattern)
-		} else if strings.HasPrefix(line, "**/") {
-			// Add the pattern as is, and without **/ so it matches in current dir
-			pattern.match, err = glob.Compile(line, '/')
-			if err != nil {
-				return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-			}
-			patterns = append(patterns, pattern)
-
-			line = line[3:]
-			pattern.pattern = line
-			pattern.match, err = glob.Compile(line, '/')
-			if err != nil {
-				return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-			}
-			patterns = append(patterns, pattern)
-		} else {
-			// Path name or pattern, add it so it matches files both in
-			// current directory and subdirs.
-			pattern.match, err = glob.Compile(line, '/')
-			if err != nil {
-				return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-			}
-			patterns = append(patterns, pattern)
-
-			line := "**/" + line
-			pattern.pattern = line
-			pattern.match, err = glob.Compile(line, '/')
-			if err != nil {
-				return errors.Wrapf(err, "invalid pattern %q in ignore file", line)
-			}
-			patterns = append(patterns, pattern)
 		}
+		patterns = append(patterns, newPatterns...)
 		return nil
 	}
 

lib/ignore/ignore_test.go

@@ -1098,3 +1098,55 @@ func TestPartialIncludeLine(t *testing.T) {
 		}
 	}
 }
+
+func TestSkipIgnoredDirs(t *testing.T) {
+	tcs := []struct {
+		pattern  string
+		expected bool
+	}{
+		{`!/test`, true},
+		{`!/t[eih]t`, true},
+		{`!/t*t`, true},
+		{`!/t?t`, true},
+		{`!/**`, true},
+		{`!/parent/test`, true},
+		{`!/parent/t[eih]t`, true},
+		{`!/parent/t*t`, true},
+		{`!/parent/t?t`, true},
+		{`!/**.mp3`, false},
+		{`!/pa*nt/test`, false},
+		{`!/pa[sdf]nt/t[eih]t`, false},
+		{`!/lowest/pa[sdf]nt/test`, false},
+		{`!/lo*st/parent/test`, false},
+		{`/pa*nt/test`, true},
+		{`test`, true},
+		{`*`, true},
+	}
+
+	for _, tc := range tcs {
+		pats, err := parseLine(tc.pattern)
+		if err != nil {
+			t.Error(err)
+		}
+		for _, pat := range pats {
+			if got := pat.allowsSkippingIgnoredDirs(); got != tc.expected {
+				t.Errorf(`Pattern "%v": got %v, expected %v`, pat, got, tc.expected)
+			}
+		}
+	}
+
+	pats := New(fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"), WithCache(true))
+
+	stignore := `
+	/foo/ign*
+	!/f*
+	!/bar
+	*
+	`
+	if err := pats.Parse(bytes.NewBufferString(stignore), ".stignore"); err != nil {
+		t.Fatal(err)
+	}
+	if !pats.SkipIgnoredDirs() {
+		t.Error("SkipIgnoredDirs should be true")
+	}
+}

lib/scanner/walk_test.go

@@ -765,6 +765,51 @@ func TestNotExistingError(t *testing.T) {
 	}
 }
 
+func TestSkipIgnoredDirs(t *testing.T) {
+	tmp, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmp)
+
+	fss := fs.NewFilesystem(fs.FilesystemTypeBasic, tmp)
+
+	name := "foo/ignored"
+	err = fss.MkdirAll(name, 0777)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	stat, err := fss.Lstat(name)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	w := &walker{}
+
+	pats := ignore.New(fss, ignore.WithCache(true))
+
+	stignore := `
+	/foo/ign*
+	!/f*
+	*
+	`
+	if err := pats.Parse(bytes.NewBufferString(stignore), ".stignore"); err != nil {
+		t.Fatal(err)
+	}
+	if !pats.SkipIgnoredDirs() {
+		t.Error("SkipIgnoredDirs should be true")
+	}
+
+	w.Matcher = pats
+
+	fn := w.walkAndHashFiles(context.Background(), nil, nil)
+
+	if err := fn(name, stat, nil); err != fs.SkipDir {
+		t.Errorf("Expected %v, got %v", fs.SkipDir, err)
+	}
+}
+
 // Verify returns nil or an error describing the mismatch between the block
 // list and actual reader contents
 func verify(r io.Reader, blocksize int, blocks []protocol.BlockInfo) error {