doublestar: `Glob` incorrectly returns errors if pattern does not match when failing on IO errors

This code:

matches, err := doublestar.Glob(fsys, "non-existent-path", doublestar.WithFailOnIOErrors())

incorrectly returns an error if non-existent-path does not exist. It should return an empty list of matches and no error.

The exists function is certainly buggy, as it returns an error when a file does not exist, whereas it should return false and no error. It should be something like:

// Returns true if the path exists
func (g *glob) exists(fsys fs.FS, name string) (bool, error) {
	_, err := fs.Stat(fsys, name)
	if errors.Is(err, fs.ErrNotExist) { // name not existing is not an error
		return false, nil
	}
	return err == nil, g.forwardErrIfFailOnIOErrors(err)
}

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 20 (20 by maintainers)

Commits related to this issue

fixes #73 patterns that return ErrNotExist + WithFailOnIOErrors — committed to bmatcuk/doublestar by bmatcuk 2 years ago

Most upvoted comments

v4.3.1 released with this bug fixed. I also addressed the small issue @twpayne found in the PR and added some tests that cause IO errors to flex that code 😄

bmatcuk on Nov 5, 2022

Thank you for the speedy fix 😃

twpayne on Nov 5, 2022

Ya, I’m going to add that as a separate feature… I don’t want to overload this PR with too much.

@twpayne do you have any input on my PR?

bmatcuk on Nov 4, 2022

The solution was actually a bit more involved. I’ve opened a PR and I’d appreciate if you both gave it a quick test to make sure it works the way you expect before I merge it.

One caveat is that, because of poor symlink support in io.fs, I cannot differentiate broken symlinks from other nonexistent paths. In other words, I must treat a pattern such as nonexistent/* the same as broken-symlink/*, so, for that reason, broken symlinks won’t return an IO errors. We can revisit that if this golang proposal is ever implemented. Not sure if either of you expected broken symlinks to return an error or not, but I wanted to mention it.

bmatcuk on Nov 3, 2022

No worries, it’s a fairly easy fix… I can handle it =)

bmatcuk on Nov 3, 2022

I agree now with @twpayne that the inconsistency is bad and confusing. The interpretation of the passed pattern parameter changes when the WithFailOnIOErrors option is passed. Without WithFailOnIOErrors the parameter is always a pattern. With WithFailOnIOErrors the parameter is interpreted as pattern and sometimes as path that must exist.

I suggest to change the behavior when using WithFailOnIOErrors to consistently return an empty list if a pattern does not match anything, instead of fs.ErrNotExist sometimes.

To still support to have a fs.ErrNotExist error when a pattern is passed that is a filepath and it does not exist, the following solutions come to my mind:

Introduce another Option that enables returning an fs.ErrNotExist error if a pattern is a path and it does not exist.
Alternatively doublestar could export a IsGlobPattern(p string) bool function. When IsGlobPattern(p), returns false, the user could only check if the path exist and then eventually return fs.ErrNotExist. If IsGlobPattern() returns true, he can call Glob() and handle an empty result as wanted.

Option 1. should have a better performance. Option 2. is more generic, there might also be other usecases for an IsGlobPattern() function. It also seems more elegant to me.

fho on Nov 2, 2022