Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981243002:
isolate: (refactor) extract blacklisting into common package (Closed)
Descriptionisolate: (refactor) extract blacklisting into common package
BUG=692940
Review-Url: https://codereview.chromium.org/2981243002
Committed: https://github.com/luci/luci-go/commit/8ffe319b319921ef4e757c2a6c8b7811e4913c10
Patch Set 1 #
Total comments: 26
Patch Set 2 : address review comments #Patch Set 3 : address review comments #
Messages
Total messages: 12 (4 generated)
mcgreevy@chromium.org changed reviewers: + tansell@chromium.org
https://codereview.chromium.org/2981243002/diff/1/client/archiver/directory_t... File client/archiver/directory_test.go (left): https://codereview.chromium.org/2981243002/diff/1/client/archiver/directory_t... client/archiver/directory_test.go:38: func TestWalkBadRegexp(t *testing.T) { This test was removed because it is now covered by filesystem_view_linux_test.go? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:22: // FilesystemView provides a very simple restricted "view" into a filesystem. 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? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:44: // or an empty string if path matches a blacklist entry. Why an empty string rather than nil? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:48: // * the basename return by filepath.Base(path). Is this a full string match or partial? IE would the regex "blah" match "ablahb"? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view_linux_test.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:1: // Copyright 2017 The LUCI Authors. Why filesystem_view_linux_test.go ? Is there a windows / mac version? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:59: wantErr: true, bad middle pattern?
https://codereview.chromium.org/2981243002/diff/1/client/archiver/directory_t... File client/archiver/directory_test.go (left): https://codereview.chromium.org/2981243002/diff/1/client/archiver/directory_t... client/archiver/directory_test.go:38: func TestWalkBadRegexp(t *testing.T) { On 2017/07/19 04:48:40, mithro wrote: > This test was removed because it is now covered by > filesystem_view_linux_test.go? Yes. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:22: // FilesystemView provides a very simple restricted "view" into a filesystem. On 2017/07/19 04:48:41, mithro wrote: > Maybe just 'FilesystemView provides a filtered "view" of a filesystem.'? Don't > really need the "very simple" bit. Done. > Is there an existing "FilesystemView" type thing which provides an unfiltered / > unmodified equivalent? No. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:44: // or an empty string if path matches a blacklist entry. On 2017/07/19 04:48:41, mithro wrote: > Why an empty string rather than nil? That would require returning an *string, rather than a string. Do you want me to do that? https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:48: // * the basename return by filepath.Base(path). On 2017/07/19 04:48:41, mithro wrote: > Is this a full string match or partial? > > IE would the regex "blah" match "ablahb"? 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. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view_linux_test.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:1: // Copyright 2017 The LUCI Authors. On 2017/07/19 04:48:41, mithro wrote: > Why filesystem_view_linux_test.go ? > > Is there a windows / mac version? The presence of "_linux" before "_test" will cause this to run only on linux. filepath.Match will have different behaviours depending on the platform. My intention here is not to test the platform-specific behaviour of the filepath.Match implementation, since that's already tested by they author of that code. But I do need to run the tests on *some* platform. I chose linux. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:59: wantErr: true, On 2017/07/19 04:48:41, mithro wrote: > bad middle pattern? I don't think that's a case worth testing.
Comments are pretty much of the form "have you considered XYZ" & nits. Thus assuming you have, LGTM. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:44: // or an empty string if path matches a blacklist entry. On 2017/07/19 05:11:04, mcgreevy wrote: > On 2017/07/19 04:48:41, mithro wrote: > > Why an empty string rather than nil? > > That would require returning an *string, rather than a string. Do you want me to > do that? 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. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:48: // * the basename return by filepath.Base(path). On 2017/07/19 05:11:04, mcgreevy wrote: > On 2017/07/19 04:48:41, mithro wrote: > > Is this a full string match or partial? > > > > IE would the regex "blah" match "ablahb"? > > 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. Ack - Looks okay. I might change "Blacklist entries are matched" to Blacklist globs are matched..." https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view_linux_test.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:1: // Copyright 2017 The LUCI Authors. On 2017/07/19 05:11:04, mcgreevy wrote: > On 2017/07/19 04:48:41, mithro wrote: > > Why filesystem_view_linux_test.go ? > > > > Is there a windows / mac version? > > The presence of "_linux" before "_test" will cause this to run only on linux. > filepath.Match will have different behaviours depending on the platform. My > intention here is not to test the platform-specific behaviour of the > filepath.Match implementation, since that's already tested by they author of > that code. But I do need to run the tests on *some* platform. I chose linux. Are blacklist patterns OS dependent? I feel like there could be possible interactions between filepath.Match for blacklisting and the relative path conversion -- specially around driver letters and path separators. Especially where both things are working as expected but the result is _not_ what was expected. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:59: wantErr: true, On 2017/07/19 05:11:04, mcgreevy wrote: > On 2017/07/19 04:48:41, mithro wrote: > > bad middle pattern? > > I don't think that's a case worth testing. If you don't really think it is worth testing, I'm okay with you not adding it. I'm not sure about Go, but in many other languages 0, 1, 2 and "many" items are all worth checking. The "2 element" case tends to be different from the "3 element" case if the first & last element cases are both special cases (which is not uncommon) it doesn't test the "non-special case". In Python it is also pretty easy to accidentally do a "return a[0], a[1]" rather than a "return a[0], a[-1]" or a "return a[0], a[1:]" which could pass a lot of these cases accidentally :-) https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:142: func TestAppliesBlacklist(t *testing.T) { BTW Does blacklisting (via filepath.Match) support the full globbing syntax? I might think about add a test which makes sure that escaping isn't some getting mangled in some way. This probably isn't a common problem in Go but certainly in bash scripts it comes up a lot... There might also be something around on Windows where the file path separator is `\` and escaping / matching. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:217: So(err, ShouldBeNil) My mind keeps reading this as "Should Be Neil" :-P https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:220: So(relPath, ShouldEqual, tc.wantRelPath) It "feels" like you be checking err value first and then comparing relPath. Your comment explains why it is perfectly safe to do things in this order, but that comes *after* you have red the potentially "unsafe code".
https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:44: // or an empty string if path matches a blacklist entry. On 2017/07/19 05:46:18, mithro wrote: > On 2017/07/19 05:11:04, mcgreevy wrote: > > On 2017/07/19 04:48:41, mithro wrote: > > > Why an empty string rather than nil? > > > > That would require returning an *string, rather than a string. Do you want me > to > > do that? > > 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. 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. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view.go:48: // * the basename return by filepath.Base(path). On 2017/07/19 05:46:18, mithro wrote: > On 2017/07/19 05:11:04, mcgreevy wrote: > > On 2017/07/19 04:48:41, mithro wrote: > > > Is this a full string match or partial? > > > > > > IE would the regex "blah" match "ablahb"? > > > > 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. > > Ack - Looks okay. > > I might change "Blacklist entries are matched" to Blacklist globs are > matched..." I did... https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... File client/internal/common/filesystem_view_linux_test.go (right): https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:1: // Copyright 2017 The LUCI Authors. On 2017/07/19 05:46:18, mithro wrote: > On 2017/07/19 05:11:04, mcgreevy wrote: > > On 2017/07/19 04:48:41, mithro wrote: > > > Why filesystem_view_linux_test.go ? > > > > > > Is there a windows / mac version? > > > > The presence of "_linux" before "_test" will cause this to run only on linux. > > filepath.Match will have different behaviours depending on the platform. My > > intention here is not to test the platform-specific behaviour of the > > filepath.Match implementation, since that's already tested by they author of > > that code. But I do need to run the tests on *some* platform. I chose linux. > > Are blacklist patterns OS dependent? > > I feel like there could be possible interactions between filepath.Match for > blacklisting and the relative path conversion -- specially around driver letters > and path separators. Especially where both things are working as expected but > the result is _not_ what was expected. In practice, blacklists look to be pretty simple, either the defaults: https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/client/... or a simple list of file names: https://cs.chromium.org/chromium/src/third_party/skia/infra/bots/ct/blacklist... and in fact, the --blacklist flag for the archive command incorrectly describes the patterns as regexps when they're actually globs. So this code is already brittle in the sense that it is under- and incorrectly-documented, and I don't intend to spend a lot of time improving it. I'll fix the documentation of the blacklist flag and leave it at that. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:59: wantErr: true, On 2017/07/19 05:46:19, mithro wrote: > On 2017/07/19 05:11:04, mcgreevy wrote: > > On 2017/07/19 04:48:41, mithro wrote: > > > bad middle pattern? > > > > I don't think that's a case worth testing. > > If you don't really think it is worth testing, I'm okay with you not adding it. > > I'm not sure about Go, but in many other languages 0, 1, 2 and "many" items are > all worth checking. The "2 element" case tends to be different from the "3 > element" case if the first & last element cases are both special cases (which is > not uncommon) it doesn't test the "non-special case". > > In Python it is also pretty easy to accidentally do a "return a[0], a[1]" rather > than a "return a[0], a[-1]" or a "return a[0], a[1:]" which could pass a lot of > these cases accidentally :-) The main failure mode I'm trying to cover here is where the implementation determines whether there was a blacklist match based only on a single element from the blacklist, rather than all of them. I think this covers it. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:142: func TestAppliesBlacklist(t *testing.T) { On 2017/07/19 05:46:19, mithro wrote: > BTW Does blacklisting (via filepath.Match) support the full globbing syntax? > > I might think about add a test which makes sure that escaping isn't some getting > mangled in some way. This probably isn't a common problem in Go but certainly in > bash scripts it comes up a lot... > > There might also be something around on Windows where the file path separator is > `\` and escaping / matching. As mentioned in a comment above, this feature is already pretty badly documented (the doc for the flag said regex instead of glob), and I don't propose to go through and fix everything that's wrong with it. I'm just trying not to break anything that's currently using it, and adding tests to ensure that my refactoring is not causing any new breakage. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:217: So(err, ShouldBeNil) On 2017/07/19 05:46:19, mithro wrote: > My mind keeps reading this as "Should Be Neil" :-P Acknowledged. https://codereview.chromium.org/2981243002/diff/1/client/internal/common/file... client/internal/common/filesystem_view_linux_test.go:220: So(relPath, ShouldEqual, tc.wantRelPath) On 2017/07/19 05:46:19, mithro wrote: > It "feels" like you be checking err value first and then comparing relPath. Your > comment explains why it is perfectly safe to do things in this order, but that > comes *after* you have red the potentially "unsafe code". Done.
Thanks for the response! Still LGTM!
The CQ bit was checked by mcgreevy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500611474378870,
"parent_rev": "3cafc370cbed08c47b8cc67dc1f2b72f23b423a9", "commit_rev":
"8ffe319b319921ef4e757c2a6c8b7811e4913c10"}
Message was sent while issue was closed.
Description was changed from ========== isolate: (refactor) extract blacklisting into common package BUG=692940 ========== to ========== isolate: (refactor) extract blacklisting into common package BUG=692940 Review-Url: https://codereview.chromium.org/2981243002 Committed: https://github.com/luci/luci-go/commit/8ffe319b319921ef4e757c2a6c8b7811e4913c10 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-go/commit/8ffe319b319921ef4e757c2a6c8b7811e4913c10 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
