Tiny cleanup I noticed while trying to fix/test another issue
(https://github.com/syncthing/syncthing/pull/9600). I shortly tried to
figure out what it was used for in the past, but gave up without
results.
Previously we queried cache with backslashes, and stored entries with
slashes. As in no cache hits ever for non-toplevel files. I also
eventually remembered that cache is disabled by default, so this is a
bit pointless, but still right :P
This improves the ignore handling so that directories can be fully
ignored (skipped in the watcher) in more cases. Specifically, where the
previous rule was that any complex `!`-pattern would disable skipping
directories, the new rule is that only matches on patterns *after* such
a `!`-pattern disable skipping. That is, the following now does the
intuitive thing:
```
/foo
/bar
!whatever
*
```
- `/foo/**` and `/bar/**` are completely skipped, since there is no
chance anything underneath them could ever be not-ignored
- `!whatever` toggles the "can't skip directories any more" flag
- Anything that matches `*` can't skip directories, because it's
possible we can have `whatever` match something deeper.
To enable this, some refactoring was necessary:
- The "can skip dirs" flag is now a property of the match result, not of
the pattern set as a whole.
- That meant returning a boolean is not good enough, we need to actually
return the entire `Result` (or, like, two booleans but that seemed
uglier and more annoying to use)
- `ShouldIgnore(string) boolean` went away with
`Match(string).IsIgnored()` being the obvious replacement (API
simplification!)
- The watcher then needed to import the `ignore` package (for the
`Result` type), but `fs` imports the watcher and `ignore` imports `fs`.
That's a cycle, so I broke out `Result` into a package of its own so
that it can be safely imported everywhere in things like `type Matcher
interface { Match(string) result.Result }`. There's a fair amount of
stuttering in `result.Result` and maybe we should go with something like
`ignoreresult.R` or so, leaving this open for discussion.
Tests refactored to suit, I think this change is in fact quite well
covered by the existing ones...
Also some noise because a few of the changed files were quite old and
got the `gofumpt` treatment by my editor. Sorry not sorry.
---------
Co-authored-by: Simon Frei <freisim93@gmail.com>
This fixes various test issues with Go 1.20.
- Most tests rewritten to use fakefs where possible
- Some tests that were already skipped, or dubious (invasive,
unmaintainable, unclear what they even tested) have been removed
- Some actual code rewritten to better support testing in fakefs
Co-authored-by: Eric P <eric@kastelo.net>
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.
all: Add package runtimeos for runtime.GOOS comparisons
I grew tired of hand written string comparisons. This adds generated
constants for the GOOS values, and predefined Is$OS constants that can
be iffed on. In a couple of places I rewrote trivial switch:es to if:s,
and added Illumos where we checked for Solaris (because they are
effectively the same, and if we're going to target one of them that
would be Illumos...).
This commit replaces `os.MkdirTemp` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.
Prior to this commit, temporary directory created using `os.MkdirTemp`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Fatal(err)
}
}
is also tedious, but `t.TempDir` handles this for us nicely.
Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
This is a mostly pointless change to make security scanners and static
analysis tools happy, as they all hate seeing md5. None of our md5 uses
were security relevant, but still. Only visible effect of this change is
that our temp file names for very long file names become slightly longer
than they were previously...
This changes the error handling in loading ignores slightly:
- There is a new ParseError type that is returned as the error
(somewhere in the chain) when the problem was not an I/O error loading
the file, but some issue with the contents.
- If the file was read successfully but not parsed successfully we still
return the lines read (in addition to nil patterns and a ParseError).
- In the API, if the error IsParseError then we return a successful
HTTP response with the lines and the actual error included in the JSON
object.
- In the GUI, as long as the HTTP call to load the ignores was
successful we can edit the ignores. If there was an error we show this
as a validation error on the dialog.
Also some cleanup on the Javascript side as it for some reason used
jQuery instead of Angular for this editor...
- In the few places where we wrap errors, use the new Go 1.13 "%w"
construction instead of %s or %v.
- Where we create errors with constant strings, consistently use
errors.New and not fmt.Errorf.
- Remove capitalization from errors in the few places where we had that.
Otherwise all the lines from includes will be shown in the web UI instead of
just the #include ... line. This problem was introduced in #3996.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4248
LGTM: calmh
This solves the erratic test failures on model.TestIgnores by ensuring
that the ignore patterns are reloaded even in the face of unchanged
timestamps.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4208
