[fix] change behaviour of FindFiles to ignore errors

[mahadzaryab1]

• Replaced filepath.Walk with the newer fs.WalkDir
• Changed the behaviour of FindFiles to only return an error if no paths were found
• Fixes FindFiles exits and returns no files when it can't open a directory #99

[mahadzaryab1]

@bitfield Do you have any thoughts on how we should test the case where there is an error but we get more than one path?

[bitfield]

That's a tricky one! What about creating a tempdir and then setting up files a, b, and c within it, of which the permissions of c are 0o000? Would that work?

[mahadzaryab1]

That's a tricky one! What about creating a tempdir and then setting up files a, b, and c within it, of which the permissions of c are 0o000? Would that work?

@bitfield I gave it a shot but I can't seem to produce that case. Changing the permissions of the file doesn't prevent the path from being listed. Let me know if you have any thoughts on how to proceed.

[bitfield]

Remember fs.WalkDir only calls your func for directories. It doesn't try to read files, so you won't ever receive a non-nil error for permissions on a file, only on a directory.

[mahadzaryab1]

@bitfield added a test case - can you take another look?

[bitfield]

This tempdir will be deleted anyway.

[mahadzaryab1]

@bitfield I need to do this so that this dir can get cleaned up. Otherwise, the cleanup function throws a permissions error. Let me know if you'd like to do this a different way.

TempDir RemoveAll cleanup: openfdat /var/folders/mn/31m4g_6j7qz26m7_00mgsbrc0000gn/T/TestFindFiles_DoesNotErrorWhenSubDirectoryIsNotReadable1455531535/001/restricted_dir: permission denied

[bitfield]

That's because you're also creating a file inside the restricted directory. So RemoveAll isn't able to delete the file, because that requires modifying the directory, which you don't have permission to do.

You don't need this file for the test (and FindFiles won't be able to even know it exists, because of the directory permissions). Without the file, you don't need the Cleanup func, because the automatic RemoveAll succeeds.

[mahadzaryab1]

@bitfield If I don't add the file - then wouldn't the test pass regardless because FindFiles doesn't print directories? I was trying to show that there's a path inside the restricted dir that is not printed.

[bitfield]

Well, according to your test name, the test is about whether or not FindFiles returns an error if it encounters one on its walk. If the error happens to be "can't list files in directory", then we hardly need to test that FindFIles doesn't return those files: how could it? Testing the impossible doesn't catch bugs (testing assumptions, on the other hand, is an excellent idea and we should always do it).

[mahadzaryab1]

@bitfield got it - thanks for the explanation! i removed the file for the test

[bitfield]

This test covers the case where the induced error is on the last path that FindFiles visits. Unfortunately, this just happens to miss a logic bug in the implementation of FindFiles. If you change the test (for example, renaming restricted_dir to a_restricted_dir, so that it's visited before file_a), you'll see what I mean.

[mahadzaryab1]

This test covers the case where the induced error is on the last path that FindFiles visits. Unfortunately, this just happens to miss a logic bug in the implementation of FindFiles. If you change the test (for example, renaming restricted_dir to a_restricted_dir, so that it's visited before file_a), you'll see what I mean.

Ah okay I see. I added a check in FindFiles to skip the directory if its a permission error. What do you think?

[bitfield]

Well, I don't think it matters what the error is. The point is that if we encounter any errors during the walk, we should only report them if we haven't also found some valid files.

[mahadzaryab1]

Well, I don't think it matters what the error is. The point is that if we encounter any errors during the walk, we should only report them if we haven't also found some valid files.

I see - should we then always return fs.SkipDir when we encounter an error during the walk?

[bitfield]

should we then always return fs.SkipDir when we encounter an error during the walk?

Well, there are only four possibilities:

1. Return SkipAll - that's no good, because it stops walking altogether
2. Return the error - ditto
3. Return nil - no good, because it loses the error
4. Return SkipDir

So I don't think we actually have a choice to make here, unless there's another possibility you can think of?

[mahadzaryab1]

should we then always return fs.SkipDir when we encounter an error during the walk?

Well, there are only four possibilities:

1. Return SkipAll - that's no good, because it stops walking altogether
2. Return the error - ditto
3. Return nil - no good, because it loses the error
4. Return SkipDir

So I don't think we actually have a choice to make here, unless there's another possibility you can think of?

@bitfield Good point. But if we always run SkipDir, then we lose the error as well because WalkDir will swallow the error. For example, this test would fail:

func TestFindFiles_InNonexistentPathReturnsError(t *testing.T) {
	t.Parallel()
	p := script.FindFiles("nonexistent_path")
	if p.Error() == nil {
		t.Fatal("want error for nonexistent path")
	}
}

[bitfield]

Right. But the WalkFunc can be a closure, so we can save the error in some variable in the outer scope before returning SkipDir.

[mahadzaryab1]

Right. But the WalkFunc can be a closure, so we can save the error in some variable in the outer scope before returning SkipDir.

@bitfield Addressed. Let me know what you think.

[bitfield]

Great! This code looks solid. Would you like to update the documentation a little to explain this (sensible, but non-obvious) error behaviour?