lib/ignore, lib/model: Use an interface to detect file changes, improving tests

This solves the erratic test failures on model.TestIgnores by ensuring
that the ignore patterns are reloaded even in the face of unchanged
timestamps.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4208

lib/ignore/ignore.go

@@ -64,24 +64,59 @@ func (r Result) IsCaseFolded() bool {
 	return r&resultFoldCase == resultFoldCase
 }
 
-type Matcher struct {
-	lines     []string
-	patterns  []Pattern
-	withCache bool
-	matches   *cache
-	curHash   string
-	stop      chan struct{}
-	modtimes  map[string]time.Time
-	mut       sync.Mutex
+// The ChangeDetector is responsible for determining if files have changed
+// on disk. It gets told to Remember() files (name and modtime) and will
+// then get asked if a file has been Seen() (i.e., Remember() has been
+// called on it) and if any of the files have Changed(). To forget all
+// files, call Reset().
+type ChangeDetector interface {
+	Remember(name string, modtime time.Time)
+	Seen(name string) bool
+	Changed() bool
+	Reset()
 }
 
-func New(withCache bool) *Matcher {
-	m := &Matcher{
-		withCache: withCache,
-		stop:      make(chan struct{}),
-		mut:       sync.NewMutex(),
+type Matcher struct {
+	lines          []string
+	patterns       []Pattern
+	withCache      bool
+	matches        *cache
+	curHash        string
+	stop           chan struct{}
+	changeDetector ChangeDetector
+	mut            sync.Mutex
+}
+
+// An Option can be passed to New()
+type Option func(*Matcher)
+
+// WithCache enables or disables lookup caching. The default is disabled.
+func WithCache(v bool) Option {
+	return func(m *Matcher) {
+		m.withCache = v
 	}
-	if withCache {
+}
+
+// WithChangeDetector sets a custom ChangeDetector. The default is to simply
+// use the on disk modtime for comparison.
+func WithChangeDetector(cd ChangeDetector) Option {
+	return func(m *Matcher) {
+		m.changeDetector = cd
+	}
+}
+
+func New(opts ...Option) *Matcher {
+	m := &Matcher{
+		stop: make(chan struct{}),
+		mut:  sync.NewMutex(),
+	}
+	for _, opt := range opts {
+		opt(m)
+	}
+	if m.changeDetector == nil {
+		m.changeDetector = newModtimeChecker()
+	}
+	if m.withCache {
 		go m.clean(2 * time.Hour)
 	}
 	return m
@@ -91,7 +126,7 @@ func (m *Matcher) Load(file string) error {
 	m.mut.Lock()
 	defer m.mut.Unlock()
 
-	if m.patternsUnchanged(file) {
+	if m.changeDetector.Seen(file) && !m.changeDetector.Changed() {
 		return nil
 	}
 
@@ -108,9 +143,8 @@ func (m *Matcher) Load(file string) error {
 		return err
 	}
 
-	m.modtimes = map[string]time.Time{
-		file: info.ModTime(),
-	}
+	m.changeDetector.Reset()
+	m.changeDetector.Remember(file, info.ModTime())
 
 	return m.parseLocked(fd, file)
 }
@@ -122,7 +156,7 @@ func (m *Matcher) Parse(r io.Reader, file string) error {
 }
 
 func (m *Matcher) parseLocked(r io.Reader, file string) error {
-	lines, patterns, err := parseIgnoreFile(r, file, m.modtimes)
+	lines, patterns, err := parseIgnoreFile(r, file, m.changeDetector)
 	// Error is saved and returned at the end. We process the patterns
 	// (possibly blank) anyway.
 
@@ -142,26 +176,6 @@ func (m *Matcher) parseLocked(r io.Reader, file string) error {
 	return err
 }
 
-// patternsUnchanged returns true if none of the files making up the loaded
-// patterns have changed since last check.
-func (m *Matcher) patternsUnchanged(file string) bool {
-	if _, ok := m.modtimes[file]; !ok {
-		return false
-	}
-
-	for filename, modtime := range m.modtimes {
-		info, err := os.Stat(filename)
-		if err != nil {
-			return false
-		}
-		if !info.ModTime().Equal(modtime) {
-			return false
-		}
-	}
-
-	return true
-}
-
 func (m *Matcher) Match(file string) (result Result) {
 	if m == nil || file == "." {
 		return resultNotMatched
@@ -284,8 +298,8 @@ func hashPatterns(patterns []Pattern) string {
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
 
-func loadIgnoreFile(file string, modtimes map[string]time.Time) ([]string, []Pattern, error) {
-	if _, ok := modtimes[file]; ok {
+func loadIgnoreFile(file string, cd ChangeDetector) ([]string, []Pattern, error) {
+	if cd.Seen(file) {
 		return nil, nil, fmt.Errorf("multiple include of ignore file %q", file)
 	}
 
@@ -299,12 +313,13 @@ func loadIgnoreFile(file string, modtimes map[string]time.Time) ([]string, []Pat
 	if err != nil {
 		return nil, nil, err
 	}
-	modtimes[file] = info.ModTime()
 
-	return parseIgnoreFile(fd, file, modtimes)
+	cd.Remember(file, info.ModTime())
+
+	return parseIgnoreFile(fd, file, cd)
 }
 
-func parseIgnoreFile(fd io.Reader, currentFile string, modtimes map[string]time.Time) ([]string, []Pattern, error) {
+func parseIgnoreFile(fd io.Reader, currentFile string, cd ChangeDetector) ([]string, []Pattern, error) {
 	var lines []string
 	var patterns []Pattern
 
@@ -371,7 +386,7 @@ func parseIgnoreFile(fd io.Reader, currentFile string, modtimes map[string]time.
 		} else if strings.HasPrefix(line, "#include ") {
 			includeRel := line[len("#include "):]
 			includeFile := filepath.Join(filepath.Dir(currentFile), includeRel)
-			includeLines, includePatterns, err := loadIgnoreFile(includeFile, modtimes)
+			includeLines, includePatterns, err := loadIgnoreFile(includeFile, cd)
 			if err != nil {
 				return fmt.Errorf("include of %q: %v", includeRel, err)
 			}
@@ -466,3 +481,41 @@ func WriteIgnores(path string, content []string) error {
 
 	return nil
 }
+
+// modtimeChecker is the default implementation of ChangeDetector
+type modtimeChecker struct {
+	modtimes map[string]time.Time
+}
+
+func newModtimeChecker() *modtimeChecker {
+	return &modtimeChecker{
+		modtimes: map[string]time.Time{},
+	}
+}
+
+func (c *modtimeChecker) Remember(name string, modtime time.Time) {
+	c.modtimes[name] = modtime
+}
+
+func (c *modtimeChecker) Seen(name string) bool {
+	_, ok := c.modtimes[name]
+	return ok
+}
+
+func (c *modtimeChecker) Reset() {
+	c.modtimes = map[string]time.Time{}
+}
+
+func (c *modtimeChecker) Changed() bool {
+	for name, modtime := range c.modtimes {
+		info, err := os.Stat(name)
+		if err != nil {
+			return true
+		}
+		if !info.ModTime().Equal(modtime) {
+			return true
+		}
+	}
+
+	return false
+}