Repository: magefile/mage
Pull request #324 (2020-12-07 00:04:39)

target: rework modtime comparison logic #323
Within the `target` package:

- Separate discovery of newest target timestamp from source inspection
- Allow source inspection functions to bail out as soon as they hit a timestamp newer than the target

```
~/p/mage/target$ go test -count=1 .
ok      github.com/magefile/mage/target 0.073s
```

DavidGamba: I love this separate approach, the only other thing that I would add to this, which is the reason I don't use mage target is because in the logs I want to see a report of the file I found that was modified. Sometimes there are complex dependency trees and knowing what triggered what is helpful.
AgentZombie: I started out trying to make it compatible with the upcoming `io/fs` interfaces to make it easy to write tests that didn't have to touch the filesystem. This would also allow user implementations that return `atime`, `ctime`, or other arbitrary times. Ultimately I saw there were already perfectly good unit tests so I got lazy. I might revisit this when `io/fs` lands and then users can inject whatever funky filesystem behavior they want.
jasonmf: Requested changes pushed
natefinch: So, coming back to this, now that I better grok why there's an expanded API surface, I'm a little more ok with it. Letting people punch in their own modtime does make the code more flexible.


Patch:
diff --git a/target/newer.go b/target/newer.go
new file mode 100644
index 0000000..9f5266c
--- /dev/null
+++ b/target/newer.go
@@ -0,0 +1,125 @@
+package target
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"time"
+)
+
+var (
+	// errNewer is an ugly sentinel error to cause filepath.Walk to abort
+	// as soon as a newer file is encountered
+	errNewer = fmt.Errorf("newer item encountered")
+)
+
+// DirNewer reports whether any item in sources is newer than the target time.
+// Sources are searched recursively and searching stops as soon as any entry
+// is newer than the target.
+func DirNewer(target time.Time, sources ...string) (bool, error) {
+	walkFn := func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.ModTime().After(target) {
+			return errNewer
+		}
+		return nil
+	}
+	for _, source := range sources {
+		source = os.ExpandEnv(source)
+		err := filepath.Walk(source, walkFn)
+		if err == nil {
+			continue
+		}
+		if err == errNewer {
+			return true, nil
+		}
+		return false, err
+	}
+	return false, nil
+}
+
+// GlobNewer performs glob expansion on each source and passes the results to
+// PathNewer for inspection. It returns the first time PathNewer encounters a
+// newer file
+func GlobNewer(target time.Time, sources ...string) (bool, error) {
+	for _, g := range sources {
+		files, err := filepath.Glob(g)
+		if err != nil {
+			return false, err
+		}
+		if len(files) == 0 {
+			return false, fmt.Errorf("glob didn't match any files: %s", g)
+		}
+		newer, err := PathNewer(target, files...)
+		if err != nil {
+			return false, err
+		}
+		if newer {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// PathNewer checks whether any of the sources are newer than the target time.
+// It stops at the first newer file it encounters. Each source path is passed
+// through os.ExpandEnv.
+func PathNewer(target time.Time, sources ...string) (bool, error) {
+	for _, source := range sources {
+		source = os.ExpandEnv(source)
+		stat, err := os.Stat(source)
+		if err != nil {
+			return false, err
+		}
+		if stat.ModTime().After(target) {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// OldestModTime recurses a list of target filesystem objects and finds the
+// the oldest ModTime among them.
+func OldestModTime(targets ...string) (time.Time, error) {
+	t := time.Now().Add(time.Hour * 100000)
+	for _, target := range targets {
+		walkFn := func(_ string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			mTime := info.ModTime()
+			if mTime.Before(t) {
+				t = mTime
+			}
+			return nil
+		}
+		if err := filepath.Walk(target, walkFn); err != nil {
+			return t, err
+		}
+	}
+	return t, nil
+}
+
+// NewestModTime recurses a list of target filesystem objects and finds the
+// the newest ModTime among them.
+func NewestModTime(targets ...string) (time.Time, error) {
+	t := time.Time{}
+	for _, target := range targets {
+		walkFn := func(_ string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			mTime := info.ModTime()
+			if mTime.After(t) {
+				t = mTime
+			}
+			return nil
+		}
+		if err := filepath.Walk(target, walkFn); err != nil {
+			return t, err
+		}
+	}
+	return t, nil
+}
diff --git a/target/target.go b/target/target.go
index 2137036..79431c1 100644
--- a/target/target.go
+++ b/target/target.go
@@ -1,22 +1,9 @@
 package target
 
 import (
-	"errors"
 	"os"
-	"path/filepath"
-	"time"
 )
 
-// expand takes a collection of sources as strings, and for each one, it expands
-// environment variables in the form $FOO or ${FOO} using os.ExpandEnv
-func expand(sources []string) []string {
-	for i, s := range sources {
-		// first expand the environment
-		sources[i] = os.ExpandEnv(s)
-	}
-	return sources
-}
-
 // Path first expands environment variables like $FOO or ${FOO}, and then
 // reports if any of the sources have been modified more recently than the
 // destination. Path does not descend into directories, it literally just checks
@@ -31,16 +18,7 @@ func Path(dst string, sources ...string) (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	srcTime := stat.ModTime()
-	dt, err := loadTargets(expand(sources))
-	if err != nil {
-		return false, err
-	}
-	t := dt.modTime()
-	if t.After(srcTime) {
-		return true, nil
-	}
-	return false, nil
+	return PathNewer(stat.ModTime(), sources...)
 }
 
 // Glob expands each of the globs (file patterns) into individual sources and
@@ -50,127 +28,37 @@ func Path(dst string, sources ...string) (bool, error) {
 // environment variables before globbing -- env var expansion happens during
 // the call to Path. It is an error for any glob to return an empty result.
 func Glob(dst string, globs ...string) (bool, error) {
-	for _, g := range globs {
-		files, err := filepath.Glob(g)
-		if err != nil {
-			return false, err
-		}
-		if len(files) == 0 {
-			return false, errors.New("glob didn't match any files: " + g)
-		}
-		// it's best to evaluate each glob as we do it
-		// because we may be able to early-exit
-		shouldDo, err := Path(dst, files...)
-		if err != nil {
-			return false, err
-		}
-		if shouldDo {
-			return true, nil
-		}
+	stat, err := os.Stat(os.ExpandEnv(dst))
+	if os.IsNotExist(err) {
+		return true, nil
+	}
+	if err != nil {
+		return false, err
 	}
-	return false, nil
+	return GlobNewer(stat.ModTime(), globs...)
 }
 
-// Dir reports whether any of the sources have been modified more recently than
-// the destination.  If a source or destination is a directory, modtimes of
-// files under those directories are compared instead.  If the destination file
-// doesn't exist, it always returns true and nil.  It's an error if any of the
-// sources don't exist.
+// Dir reports whether any of the sources have been modified more recently
+// than the destination. If a source or destination is a directory, this
+// function returns true if a source has any file that has been modified more
+// recently than the most recently modified file in dst. If the destination
+// file doesn't exist, it always returns true and nil.  It's an error if any
+// of the sources don't exist.
 func Dir(dst string, sources ...string) (bool, error) {
-	stat, err := os.Stat(os.ExpandEnv(dst))
+	dst = os.ExpandEnv(dst)
+	stat, err := os.Stat(dst)
 	if os.IsNotExist(err) {
 		return true, nil
 	}
 	if err != nil {
 		return false, err
 	}
-	srcTime := stat.ModTime()
+	destTime := stat.ModTime()
 	if stat.IsDir() {
-		srcTime, err = calDirModTimeRecursive(dst, stat)
+		destTime, err = NewestModTime(dst)
 		if err != nil {
 			return false, err
 		}
 	}
-	dt, err := loadTargets(expand(sources))
-	if err != nil {
-		return false, err
-	}
-	t, err := dt.modTimeDir()
-	if err != nil {
-		return false, err
-	}
-	if t.After(srcTime) {
-		return true, nil
-	}
-	return false, nil
-}
-
-func calDirModTimeRecursive(name string, dir os.FileInfo) (time.Time, error) {
-	t := dir.ModTime()
-	ferr := filepath.Walk(name, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
-		if info.ModTime().After(t) {
-			t = info.ModTime()
-		}
-		return nil
-	})
-	if ferr != nil {
-		return time.Time{}, ferr
-	}
-	return t, nil
-}
-
-type source struct {
-	path string
-	info os.FileInfo
-}
-
-type depTargets struct {
-	src    []source
-	hasdir bool
-	latest time.Time
-}
-
-func loadTargets(targets []string) (*depTargets, error) {
-	d := &depTargets{}
-	for _, v := range targets {
-		stat, err := os.Stat(v)
-		if err != nil {
-			return nil, err
-		}
-		if stat.IsDir() {
-			d.hasdir = true
-		}
-		d.src = append(d.src, source{path: v, info: stat})
-		if stat.ModTime().After(d.latest) {
-			d.latest = stat.ModTime()
-		}
-	}
-	return d, nil
-}
-
-func (d *depTargets) modTime() time.Time {
-	return d.latest
-}
-
-func (d *depTargets) modTimeDir() (time.Time, error) {
-	if !d.hasdir {
-		return d.latest, nil
-	}
-	var err error
-	for _, src := range d.src {
-		t := src.info.ModTime()
-		if src.info.IsDir() {
-			t, err = calDirModTimeRecursive(src.path, src.info)
-			if err != nil {
-				return time.Time{}, err
-			}
-		}
-		if t.After(d.latest) {
-			d.latest = t
-		}
-	}
-	return d.latest, nil
+	return DirNewer(destTime, sources...)
 }


Test patch:
diff --git a/target/newer_test.go b/target/newer_test.go
new file mode 100644
index 0000000..f797437
--- /dev/null
+++ b/target/newer_test.go
@@ -0,0 +1,108 @@
+package target
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+)
+
+func TestNewestModTime(t *testing.T) {
+	t.Parallel()
+	dir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("error creating temp dir: %s", err.Error())
+	}
+	defer os.RemoveAll(dir)
+	for _, name := range []string{"a", "b", "c", "d"} {
+		out := filepath.Join(dir, name)
+		if err := ioutil.WriteFile(out, []byte("hi!"), 0644); err != nil {
+			t.Fatalf("error writing file: %s", err.Error())
+		}
+	}
+	time.Sleep(10 * time.Millisecond)
+	outName := filepath.Join(dir, "c")
+	outfh, err := os.OpenFile(outName, os.O_APPEND|os.O_WRONLY, 0644)
+	if err != nil {
+		t.Fatalf("error opening file to append: %s", err.Error())
+	}
+	if _, err := outfh.WriteString("\nbye!\n"); err != nil {
+		t.Fatalf("error appending to file: %s", err.Error())
+	}
+	if err := outfh.Close(); err != nil {
+		t.Fatalf("error closing file: %s", err.Error())
+	}
+
+	afi, err := os.Stat(filepath.Join(dir, "a"))
+	if err != nil {
+		t.Fatalf("error stating unmodified file: %s", err.Error())
+	}
+
+	cfi, err := os.Stat(outName)
+	if err != nil {
+		t.Fatalf("error stating modified file: %s", err.Error())
+	}
+	if afi.ModTime().Equal(cfi.ModTime()) {
+		t.Fatal("modified and unmodified file mtimes equal")
+	}
+
+	newest, err := NewestModTime(dir)
+	if err != nil {
+		t.Fatalf("error finding newest mod time: %s", err.Error())
+	}
+	if !newest.Equal(cfi.ModTime()) {
+		t.Fatal("expected newest mod time to match c")
+	}
+}
+
+func TestOldestModTime(t *testing.T) {
+	t.Parallel()
+	dir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("error creating temp dir: %s", err.Error())
+	}
+	defer os.RemoveAll(dir)
+	for _, name := range []string{"a", "b", "c", "d"} {
+		out := filepath.Join(dir, name)
+		if err := ioutil.WriteFile(out, []byte("hi!"), 0644); err != nil {
+			t.Fatalf("error writing file: %s", err.Error())
+		}
+	}
+	time.Sleep(10 * time.Millisecond)
+	for _, name := range []string{"a", "b", "d"} {
+		outName := filepath.Join(dir, name)
+		outfh, err := os.OpenFile(outName, os.O_APPEND|os.O_WRONLY, 0644)
+		if err != nil {
+			t.Fatalf("error opening file to append: %s", err.Error())
+		}
+		if _, err := outfh.WriteString("\nbye!\n"); err != nil {
+			t.Fatalf("error appending to file: %s", err.Error())
+		}
+		if err := outfh.Close(); err != nil {
+			t.Fatalf("error closing file: %s", err.Error())
+		}
+	}
+
+	afi, err := os.Stat(filepath.Join(dir, "a"))
+	if err != nil {
+		t.Fatalf("error stating unmodified file: %s", err.Error())
+	}
+
+	outName := filepath.Join(dir, "c")
+	cfi, err := os.Stat(outName)
+	if err != nil {
+		t.Fatalf("error stating modified file: %s", err.Error())
+	}
+	if afi.ModTime().Equal(cfi.ModTime()) {
+		t.Fatal("modified and unmodified file mtimes equal")
+	}
+
+	newest, err := OldestModTime(dir)
+	if err != nil {
+		t.Fatalf("error finding oldest mod time: %s", err.Error())
+	}
+	if !newest.Equal(cfi.ModTime()) {
+		t.Fatal("expected newest mod time to match c")
+	}
+}
diff --git a/target/target_test.go b/target/target_test.go
index 1f2acff..70f8dea 100644
--- a/target/target_test.go
+++ b/target/target_test.go
@@ -422,6 +422,18 @@ func TestDir(t *testing.T) {
 			sources: []string{"$MYDIR"},
 			expect:  true,
 		},
+		{
+			desc:    "Source file is newer than dst dir",
+			target:  "dir/dir2",
+			sources: []string{"file_five"},
+			expect:  true,
+		},
+		{
+			desc:    "Source file is not newer than dst dir",
+			target:  "dir/dir2",
+			sources: []string{"file_one"},
+			expect:  false,
+		},
 	}
 
 	for _, c := range table {