lib/ignore: properly handle non-existing included ignore-files (fixes #8764) #8874
Conversation
…ixes syncthing#8764) In the sequence of loading ignores, the error File Does Not Exist is not being considered a fatal error, since the .stignore file is allowed to not exist. However, included ignore files also tossed that same error in case those do not exist while in those cases it's considered an error and it should lead to the folder stopping. Changing the error when opening an included ignore file to something other than the regular does fix this issue, as in it now works again as described in the Documentation.
|
I'll actually double-check the related test-case |
lib/ignore/ignore.go
Outdated
| fd, info, err := loadIgnoreFile(filesystem, file) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, parseError(fmt.Errorf("could not load included ignore file: %v", err)) |
There was a problem hiding this comment.
Should use to use %w to preserve erro chain. And the inner error already signifies that something went wrong, the wrapping context could be more terse to shrink the overall length:
| return nil, parseError(fmt.Errorf("could not load included ignore file: %v", err)) | |
| return nil, parseError(fmt.Errorf("included ignore file: %w", err)) |
There was a problem hiding this comment.
Actually the error is already wrapped in the calling function, so I'd say no wrapping/context is needed here at all:
syncthing/lib/ignore/ignore.go
Line 561 in 3d94efd
There was a problem hiding this comment.
So there should be a comment to this effect, but the entire point of this fix is to not preserve the error chain, because we treat fs.NotExist as a non-error in other places (.stignore may not exist). We need to differentiate between .stignore not existing (fine) and some #include file not existing (not fine).
Alternatively, Load() could handle this and return nil when there is no .stignore, but that also seems weird and might have effects I can't foresee right now...
There was a problem hiding this comment.
Ah got it, thanks for the explanation. Maybe instead of the comment make it explicit by checking:
| return nil, parseError(fmt.Errorf("could not load included ignore file: %v", err)) | |
| if fs.IsNotExist(err) { | |
| err = errors.New("missing included file") | |
| } | |
| return nil, parseError(err) |
I am fine either way, just a suggestion.
Co-authored-by: Jakob Borg <jakob@kastelo.net>
|
(mac build error is a signing issue on the builder, I'll fix that at some point...) |
|
|
||
| if cd.Seen(filesystem, file) { | ||
| return nil, parseError(fmt.Errorf("multiple include of ignore file %q", file)) | ||
| return nil, errors.New("multiple include") |
There was a problem hiding this comment.
Why shouldn't this be a parse error?
There was a problem hiding this comment.
For the same reason as why there was no need to put the initially changed error inside a parseError, the returning error from this function is always being wrapped inside a parseError already;
err = parseError(fmt.Errorf("failed to load include file %s: %w", includeFile, err)) . I don't think it adds much at this point?
There was a problem hiding this comment.
Thanks. (Damn I am thinking slowly (to the point of not thinking) today).
* main: build(deps): bump github.com/quic-go/quic-go from 0.33.0 to 0.34.0 (syncthing#8877) gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871)
* main: gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871)
* main: (42 commits) all: Correct various typos (syncthing#8870) gui, man, authors: Update docs, translations, and contributors lib/model: Set platform data, incl. copying ownership, for new folders w/ NoPermissions flag (syncthing#8883) gui: Add Thai (th) translation template. (syncthing#8887) build: Produce nightly release builds gui, man, authors: Update docs, translations, and contributors build: Bump actions version; fix Node 12 deprecation warning (syncthing#8881) build: Build Debian packages build: Sign for upgrades build: Notarize mac builds build(deps): bump github.com/quic-go/quic-go from 0.33.0 to 0.34.0 (syncthing#8877) gui, man, authors: Update docs, translations, and contributors lib/ignore: Properly handle non-existing included ignore-files (fixes syncthing#8764) (syncthing#8874) lib/connections: Avoid using nil lanChecker gui, man, authors: Update docs, translations, and contributors lib/config, lib/connections: Configurable protocol priority (ref syncthing#8626) (syncthing#8868) build: Upgrade recli (fixes syncthing#8503) (syncthing#8871) build: Update dependencies (syncthing#8869) lib/model: Improve path generation for auto accepted folders (fixes syncthing#8859) (syncthing#8860) docs: fix typo (syncthing#8857) ...
In the sequence of loading ignores, the error
no such file or directoryis not being considered a fatal error, since the.stignorefile is allowed to not exist. However, included ignore files also tossed that same error in case those did not exist while in those cases it's actually considered an error that should impact the running-status of the folder. Changing the error when opening an included ignore file to something manually defined does fix this issue, as in it now works again as described in the Documentation.Testing
Add/edit a folder. Add ignore-lines with
#include .somethingnonexistingand it should now error out properly and the folder should be stopped, instead of having the ignores not being loaded and the folder remain in a running state.