isolate: (refactor) extract blacklisting into common package

client/internal/common/filesystem_view.go

+// Copyright 2017 The LUCI Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package common
+
+import (
+        "fmt"
+        "path/filepath"
+)
+
+// FilesystemView provides a very simple restricted "view" into a filesystem.

[mithro, 2017/07/19 04:48:41]

Maybe just 'FilesystemView provides a filtered "view" of a filesystem.'? Don't
really need the "very simple" bit.

Is there an existing "FilesystemView" type thing which provides an unfiltered /
unmodified equivalent?

[mcgreevy, 2017/07/19 05:11:04]

Done.
No.

+// It translates absolute paths to relative paths based on its configured root path.
+// It also hides any paths which match a blacklist entry.
+type FilesystemView struct {
+        root      string
+        blacklist []string
+}
+
+// NewFilesystemView returns a FilesystemView based on the supplied root and blacklist, or
+// an error if blacklist contains a bad pattern.
+// root is the the base path used by RelativePath to calulate relative paths.
+// blacklist is a list of globs of files to ignore.  See RelativePath for more information.
+func NewFilesystemView(root string, blacklist []string) (FilesystemView, error) {
+        for _, b := range blacklist {
+                if _, err := filepath.Match(b, b); err != nil {
+                        return FilesystemView{}, fmt.Errorf("bad blacklist pattern \"%s\"", b)
+                }
+        }
+        return FilesystemView{root: root, blacklist: blacklist}, nil
+}
+
+// RelativePath returns a version of path which is relative to the FilesystemView root,
+// or an empty string if path matches a blacklist entry.

[mithro, 2017/07/19 04:48:41]

Why an empty string rather than nil?

[mcgreevy, 2017/07/19 05:11:04]

That would require returning an *string, rather than a string. Do you want me to
do that?

[mithro, 2017/07/19 05:46:18]

I can't think of a way that an empty string would be a valid path, so it might
be okay. You know the "go way" better then me, so happy to defer to your
judgement on is using empty string or nil with *string is better.

I wonder if you should maybe return FilesystemView.SkipFile or something in the
error value? It doesn't "feel" identical to returning io.SkipFile / io.SkipDir
from walk but just a random spitball / thought.

[mcgreevy, 2017/07/21 04:15:50]

I considered all these options (plus returning an extra skip bool value) before
settling on an empty string for relpath.  None are obviously right, and I think
that empty string is fine.

+//
+// Blacklist entries are matched against each of:
+//  * the path relative to the FilesystemView root.
+//  * the basename return by filepath.Base(path).

[mithro, 2017/07/19 04:48:41]

Is this a full string match or partial?

IE would the regex "blah" match "ablahb"?

[mcgreevy, 2017/07/19 05:11:04]

It's full match, but they're not regexes -- they're globs.  I've modified the
documentation to clarify this and point to the filepath.Match documentation for
more information.

[mithro, 2017/07/19 05:46:18]

Ack - Looks okay.

I might change "Blacklist entries are matched" to Blacklist globs are
matched..."

[mcgreevy, 2017/07/21 04:15:50]

I did...

+func (ff FilesystemView) RelativePath(path string) (string, error) {
+        relPath, err := filepath.Rel(ff.root, path)
+        if err != nil {
+                return "", fmt.Errorf("calculating relative path(%q): %v", path, err)
+        }
+        if ff.skipRelPath(relPath) {
+                relPath = ""
+        }
+        return relPath, nil
+}
+
+func (ff FilesystemView) skipRelPath(relPath string) bool {
+        // filepath.Rel is documented to call filepath.Clean on its result before returning it,
+        // which results in "." for an empty relative path.
+        if relPath == "." { // Root directory.
+                return false
+        }
+
+        for _, glob := range ff.blacklist {
+                if match(glob, relPath) || match(glob, filepath.Base(relPath)) {
+                        return true
+                }
+        }
+
+        return false
+}
+
+// match is equivalent to filepath.Match, but assumes that pattern is valid.
+func match(pattern, name string) bool {
+        matched, _ := filepath.Match(pattern, name)
+        return matched
+}