From f52ea2a708137c843d70255c19ee95d0394a90b4 Mon Sep 17 00:00:00 2001
From: mediocregopher <>
Date: Sat, 9 May 2020 18:00:00 -0600
Subject: [PATCH] Rework how FilterFilesChanged works

---
type: change
description: |-
  Rework how FilterFilesChanged works

  Previously the filter would only match if _all_ files changed matched a pattern
  within it. This ends up not being very useful, as usually the filter is being
  used to guard a specific set of content within the project, and so it's a lot
  more useful to have the filter match if _any_ files changed matched a pattern.
fingerprint: AFSaaPVOBsFYc0D3JQd0M7yJKDiS8/VjAsppPXTngGPb
credentials:
- type: pgp_signature
  pub_key_id: 95C46FA6A41148AC
  body: iQIzBAABAgAdFiEEJ6tQKp6olvZKJ0lwlcRvpqQRSKwFAl63Q/oACgkQlcRvpqQRSKxHew//QO74joMfwg+mCiSKjXRbTidYW/ILr3mspFSLMYS5RKcf2YQNpYOP24a7BINe5LQYXGAOEH7PxEWOTTJTzwv90/T/jmYwJJMuNRa3NHXfCRS63i7L2b4nWSEATPQlGZRaMLOOsYdg2m/mcEQuHgf+fmfd4iRGFJHDzxj4etcfD/cHi9fg8Assr7E8K9xoYp3vs/8cPWGP16p94LHmsu5m8pt990Bn4XzxUzHAseyjPgs3OElH67SqW9D9F2PHZvdmCo58bvt13otzn137jwf/EZQOkcOWXxaOxNG8bqZFWgFGUEQajgXYLwfPULC4J3c1Rj+umAT0R1QPzj0h2MIoE/gM1wbw5fwtIfCpM9pA526TFJfdujvYY4x8oPMve9hTQ0FZ9Vv1cTCrOWo5vVNjum0jY58GGr9/I+PXupstDmGnKHnwivz459YNq0Uuyir3KgyM8fY7rTf+FQslrv7Hafnrju3KqCvCoHT0nNj0dWVen5RFK6dee4fsYvGK8ujKYCJpwm+3/0ggJOCC3XsMOCCh/F/YMvq4Xtwo7AbNIrVPOSIAnu2khkybInmZGmIVEP4E6MWfVzjb4wVVocqYhhuHg09IDeBv9WS5s3KBUVL4RNYadrUXMOs4W7wXvHILSYx87j0UJfy9bUfALwclXaFuhBYzghbBQVN3L00ASaw=
  account: mediocregopher
---
 SPEC.md                          |  5 +--
 accessctl/filter_pattern.go      |  8 +++-
 accessctl/filter_pattern_test.go | 65 ++++++++++++++++++++++++++++++++
 accessctl/filter_test.go         |  6 +--
 4 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/SPEC.md b/SPEC.md
index d456540..9c3ffe1 100644
--- a/SPEC.md
+++ b/SPEC.md
@@ -337,10 +337,7 @@ changed between the tree of the commit's parent and the commit's tree. Matching
 is performed on the paths of the changed files, relative to the repo root.
 
 A files changed filter can have one or multiple patterns defined. The filter
-will match if at least one defined pattern matches for every file changed.
-
-(TODO this may change to be: `The filter will match if any of the changed files
-matches at least one defined pattern.`)
+will match if any of the changed files matches at least one defined pattern.
 
 A files changed filter with only one pattern can be defined like this:
 
diff --git a/accessctl/filter_pattern.go b/accessctl/filter_pattern.go
index 26ca1cd..648f804 100644
--- a/accessctl/filter_pattern.go
+++ b/accessctl/filter_pattern.go
@@ -84,9 +84,13 @@ var _ Filter = FilterFilesChanged{}
 // MatchCommit implements the method for FilterInterface.
 func (f FilterFilesChanged) MatchCommit(req CommitRequest) error {
 	for _, path := range req.FilesChanged {
-		if err := f.StringMatcher.Match(path); err != nil {
+		if err := f.StringMatcher.Match(path); errors.As(err, new(ErrFilterNoMatch)) {
+			continue
+		} else if err != nil {
 			return err
 		}
+		return nil
 	}
-	return nil
+
+	return ErrFilterNoMatch{Err: errors.New("no paths matched")}
 }
diff --git a/accessctl/filter_pattern_test.go b/accessctl/filter_pattern_test.go
index 700508f..96f0878 100644
--- a/accessctl/filter_pattern_test.go
+++ b/accessctl/filter_pattern_test.go
@@ -132,3 +132,68 @@ func TestStringMatcher(t *testing.T) {
 		})
 	}
 }
+
+func TestFilterFilesChanged(t *testing.T) {
+	mkReq := func(paths ...string) CommitRequest {
+		return CommitRequest{FilesChanged: paths}
+	}
+
+	runCommitMatchTests(t, []filterCommitMatchTest{
+		{
+			descr: "no paths",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Pattern: "foo"},
+			},
+			req:   mkReq(),
+			match: false,
+		},
+		{
+			descr: "all paths against one pattern",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Pattern: "foo/*"},
+			},
+			req:   mkReq("foo/bar", "foo/baz"),
+			match: true,
+		},
+		{
+			descr: "all paths against multiple patterns",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Patterns: []string{"foo", "bar"}},
+			},
+			req:   mkReq("foo", "bar"),
+			match: true,
+		},
+		{
+			descr: "some paths against one pattern",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Pattern: "foo"},
+			},
+			req:   mkReq("foo", "bar"),
+			match: true,
+		},
+		{
+			descr: "some paths against many patterns",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Patterns: []string{"foo", "bar"}},
+			},
+			req:   mkReq("foo", "baz"),
+			match: true,
+		},
+		{
+			descr: "no paths against one pattern",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Pattern: "foo"},
+			},
+			req:   mkReq("baz", "buz"),
+			match: false,
+		},
+		{
+			descr: "no paths against many patterns",
+			filter: FilterFilesChanged{
+				StringMatcher: StringMatcher{Patterns: []string{"foo", "bar"}},
+			},
+			req:   mkReq("baz", "buz"),
+			match: false,
+		},
+	})
+}
diff --git a/accessctl/filter_test.go b/accessctl/filter_test.go
index f6edcfa..6e3606e 100644
--- a/accessctl/filter_test.go
+++ b/accessctl/filter_test.go
@@ -12,8 +12,8 @@ type filterCommitMatchTest struct {
 	req    CommitRequest
 	match  bool
 
-	// assumes match == true, and will ensure that the returned wrapped error is
-	// this one.
+	// assumes match == false, and will ensure that the returned wrapped error
+	// is this one.
 	matchErr error
 }
 
@@ -29,7 +29,7 @@ func runCommitMatchTests(t *testing.T, tests []filterCommitMatchTest) {
 				return
 
 			} else if fErr := new(ErrFilterNoMatch); !errors.As(err, fErr) {
-				t.Fatalf("expected ErrFilterNoMatch, got %#v", err)
+				t.Fatalf("expected ErrFilterNoMatch, got: %#v", err)
 
 			} else if test.matchErr != nil && !reflect.DeepEqual(fErr.Err, test.matchErr) {
 				t.Fatalf("expected err %#v, not %#v", test.matchErr, fErr.Err)