From 946170f3fc22deb5c3f0dedf388c577083290b42 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Thu, 18 Jun 2020 11:04:00 +0200 Subject: [PATCH] gui, lib/ignore: Handle editing ignores with error (fixes #5425) (#6757) 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... --- .../syncthing/core/syncthingController.js | 35 ++++++---- .../syncthing/folder/editFolderModalView.html | 12 +++- lib/api/api.go | 18 +++-- lib/ignore/ignore.go | 68 +++++++++++++++---- lib/ignore/ignore_test.go | 19 +++++- lib/model/model.go | 10 ++- 6 files changed, 123 insertions(+), 39 deletions(-) diff --git a/gui/default/syncthing/core/syncthingController.js b/gui/default/syncthing/core/syncthingController.js index d7b7b937e..7d92be1a2 100755 --- a/gui/default/syncthing/core/syncthingController.js +++ b/gui/default/syncthing/core/syncthingController.js @@ -52,6 +52,11 @@ angular.module('syncthing.core') $scope.metricRates = false; $scope.folderPathErrors = {}; $scope.currentFolder = {}; + $scope.ignores = { + text: '', + error: null, + disabled: false, + }; resetRemoteNeed(); try { @@ -433,8 +438,8 @@ angular.module('syncthing.core') } function refreshNoAuthWarning() { - if (!$scope.system || !$scope.config) { - // We need both to be able to determine the state. + if (!$scope.system || !$scope.config || !$scope.config.gui) { + // We need all to be able to determine the state. return } @@ -1766,16 +1771,18 @@ angular.module('syncthing.core') } $scope.currentFolder.externalCommand = $scope.currentFolder.externalCommand || ""; - $('#folder-ignores textarea').val($translate.instant("Loading...")); - $('#folder-ignores textarea').attr('disabled', 'disabled'); + $scope.ignores.text = 'Loading...'; + $scope.ignores.error = null; + $scope.ignores.disabled = true; $http.get(urlbase + '/db/ignores?folder=' + encodeURIComponent($scope.currentFolder.id)) .success(function (data) { $scope.currentFolder.ignores = data.ignore || []; - $('#folder-ignores textarea').val($scope.currentFolder.ignores.join('\n')); - $('#folder-ignores textarea').removeAttr('disabled'); + $scope.ignores.text = $scope.currentFolder.ignores.join('\n'); + $scope.ignores.error = data.error; + $scope.ignores.disabled = false; }) .error(function (err) { - $('#folder-ignores textarea').val($translate.instant("Failed to load ignore patterns.")); + $scope.ignores.text = $translate.instant("Failed to load ignore patterns."); $scope.emitHTTPError(err); }); @@ -1802,8 +1809,9 @@ angular.module('syncthing.core') $scope.currentFolder = angular.copy($scope.folderDefaults); $scope.currentFolder.id = (data.random.substr(0, 5) + '-' + data.random.substr(5, 5)).toLowerCase(); $scope.currentFolder.unrelatedDevices = $scope.otherDevices(); - $('#folder-ignores textarea').val(""); - $('#folder-ignores textarea').removeAttr('disabled'); + $scope.ignores.text = ''; + $scope.ignores.error = null; + $scope.ignores.disabled = false; $scope.editFolderModal(); }); }; @@ -1818,8 +1826,9 @@ angular.module('syncthing.core') }; $scope.currentFolder.selectedDevices[device] = true; $scope.currentFolder.unrelatedDevices = $scope.otherDevices(); - $('#folder-ignores textarea').val(""); - $('#folder-ignores textarea').removeAttr('disabled'); + $scope.ignores.text = ''; + $scope.ignores.error = null; + $scope.ignores.disabled = false; $scope.editFolderModal(); }; @@ -1899,8 +1908,8 @@ angular.module('syncthing.core') delete folderCfg.versioning; } - var ignoresLoaded = !$('#folder-ignores textarea').is(':disabled'); - var ignores = $('#folder-ignores textarea').val().split('\n'); + var ignoresLoaded = !$scope.ignores.disabled; + var ignores = $scope.ignores.text.split('\n'); // Split always returns a minimum 1-length array even for no patterns if (ignores.length === 1 && ignores[0] === "") { ignores = []; diff --git a/gui/default/syncthing/folder/editFolderModalView.html b/gui/default/syncthing/folder/editFolderModalView.html index 7d53f9da0..b40340a48 100644 --- a/gui/default/syncthing/folder/editFolderModalView.html +++ b/gui/default/syncthing/folder/editFolderModalView.html @@ -9,6 +9,7 @@
  • Advanced
    • +
    @@ -43,6 +44,7 @@

    +
    @@ -82,6 +84,7 @@
    +
      Help @@ -142,9 +145,15 @@

    +

    Enter ignore patterns, one per line.

    - +
    + +

    + {{ignores.error}} +

    +

    Quick guide to supported patterns (full documentation):

    @@ -165,6 +174,7 @@ Editing {%path%}. Creating ignore patterns, overwriting an existing file at {%path%}.
    +
    diff --git a/lib/api/api.go b/lib/api/api.go index 795f40fc9..7fbc2251b 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -43,6 +43,7 @@ import ( "github.com/syncthing/syncthing/lib/discover" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/fs" + "github.com/syncthing/syncthing/lib/ignore" "github.com/syncthing/syncthing/lib/locations" "github.com/syncthing/syncthing/lib/logger" "github.com/syncthing/syncthing/lib/model" @@ -1161,15 +1162,16 @@ func (s *service) getDBIgnores(w http.ResponseWriter, r *http.Request) { folder := qs.Get("folder") - ignores, patterns, err := s.model.GetIgnores(folder) - if err != nil { + lines, patterns, err := s.model.GetIgnores(folder) + if err != nil && !ignore.IsParseError(err) { http.Error(w, err.Error(), 500) return } - sendJSON(w, map[string][]string{ - "ignore": ignores, + sendJSON(w, map[string]interface{}{ + "ignore": lines, "expanded": patterns, + "error": errorString(err), }) } @@ -1750,3 +1752,11 @@ func checkExpiry(cert tls.Certificate) error { return nil } + +func errorString(err error) *string { + if err != nil { + msg := err.Error() + return &msg + } + return nil +} diff --git a/lib/ignore/ignore.go b/lib/ignore/ignore.go index c19584482..24709cf62 100644 --- a/lib/ignore/ignore.go +++ b/lib/ignore/ignore.go @@ -10,6 +10,7 @@ import ( "bufio" "bytes" "crypto/md5" + "errors" "fmt" "io" "path/filepath" @@ -18,7 +19,6 @@ import ( "time" "github.com/gobwas/glob" - "github.com/pkg/errors" "github.com/syncthing/syncthing/lib/fs" "github.com/syncthing/syncthing/lib/osutil" @@ -40,6 +40,33 @@ func init() { } } +// A ParseError signifies an error with contents of an ignore file, +// including I/O errors on included files. An I/O error on the root level +// ignore file is not a ParseError. +type ParseError struct { + inner error +} + +func (e *ParseError) Error() string { + return fmt.Sprintf("parse error: %v", e.inner) +} + +func (e *ParseError) Unwrap() error { + return e.inner +} + +func IsParseError(err error) bool { + var e *ParseError + return errors.As(err, &e) +} + +func parseError(err error) error { + if err == nil { + return nil + } + return &ParseError{err} +} + type Pattern struct { pattern string match glob.Glob @@ -150,6 +177,10 @@ func New(fs fs.Filesystem, opts ...Option) *Matcher { return m } +// Load and parse a file. The returned error may be of type *ParseError in +// which case a file was loaded from disk but the patterns could not be +// parsed. In this case the contents of the file are nonetheless available +// in the Lines() method. func (m *Matcher) Load(file string) error { m.mut.Lock() defer m.mut.Unlock() @@ -176,6 +207,7 @@ func (m *Matcher) Load(file string) error { return err } +// Load and parse an io.Reader. See Load() for notes on the returned error. func (m *Matcher) Parse(r io.Reader, file string) error { m.mut.Lock() defer m.mut.Unlock() @@ -376,7 +408,7 @@ func loadParseIncludeFile(filesystem fs.Filesystem, file string, cd ChangeDetect } if cd.Seen(filesystem, file) { - return nil, fmt.Errorf("multiple include of ignore file %q", file) + return nil, parseError(fmt.Errorf("multiple include of ignore file %q", file)) } fd, info, err := loadIgnoreFile(filesystem, file, cd) @@ -418,7 +450,7 @@ func parseLine(line string) ([]Pattern, error) { } if line == "" { - return nil, errors.New("missing pattern") + return nil, parseError(errors.New("missing pattern")) } if pattern.result.IsCaseFolded() { @@ -431,14 +463,14 @@ func parseLine(line string) ([]Pattern, error) { if strings.HasPrefix(line, "/") { // Pattern is rooted in the current dir only pattern.match, err = glob.Compile(line[1:], '/') - return []Pattern{pattern}, err + return []Pattern{pattern}, parseError(err) } patterns := make([]Pattern, 2) if strings.HasPrefix(line, "**/") { // Add the pattern as is, and without **/ so it matches in current dir pattern.match, err = glob.Compile(line, '/') if err != nil { - return nil, err + return nil, parseError(err) } patterns[0] = pattern @@ -446,7 +478,7 @@ func parseLine(line string) ([]Pattern, error) { pattern.pattern = line pattern.match, err = glob.Compile(line, '/') if err != nil { - return nil, err + return nil, parseError(err) } patterns[1] = pattern return patterns, nil @@ -455,7 +487,7 @@ func parseLine(line string) ([]Pattern, error) { // current directory and subdirs. pattern.match, err = glob.Compile(line, '/') if err != nil { - return nil, err + return nil, parseError(err) } patterns[0] = pattern @@ -463,30 +495,36 @@ func parseLine(line string) ([]Pattern, error) { pattern.pattern = line pattern.match, err = glob.Compile(line, '/') if err != nil { - return nil, err + return nil, parseError(err) } patterns[1] = pattern return patterns, nil } func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd ChangeDetector, linesSeen map[string]struct{}) ([]string, []Pattern, error) { - var lines []string var patterns []Pattern addPattern := func(line string) error { newPatterns, err := parseLine(line) if err != nil { - return errors.Wrapf(err, "invalid pattern %q in ignore file", line) + return fmt.Errorf("invalid pattern %q in ignore file: %w", line, err) } patterns = append(patterns, newPatterns...) return nil } scanner := bufio.NewScanner(fd) - var err error + var lines []string for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) lines = append(lines, line) + } + if err := scanner.Err(); err != nil { + return nil, nil, err + } + + var err error + for _, line := range lines { if _, ok := linesSeen[line]; ok { continue } @@ -503,13 +541,13 @@ func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd Chan case strings.HasPrefix(line, "#include"): fields := strings.SplitN(line, " ", 2) if len(fields) != 2 { - err = errors.New("failed to parse #include line: no file?") + err = parseError(errors.New("failed to parse #include line: no file?")) break } includeRel := strings.TrimSpace(fields[1]) if includeRel == "" { - err = errors.New("failed to parse #include line: no file?") + err = parseError(errors.New("failed to parse #include line: no file?")) break } @@ -522,7 +560,7 @@ func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd Chan // IsNotExists(err) == true error, which we use to check // existance of the .stignore file, and just end up assuming // there is none, rather than a broken include. - err = fmt.Errorf("failed to load include file %s: %s", includeFile, err.Error()) + err = parseError(fmt.Errorf("failed to load include file %s: %w", includeFile, err)) } case strings.HasSuffix(line, "/**"): err = addPattern(line) @@ -535,7 +573,7 @@ func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd Chan } } if err != nil { - return nil, nil, err + return lines, nil, err } } diff --git a/lib/ignore/ignore_test.go b/lib/ignore/ignore_test.go index 4e743cafb..941731144 100644 --- a/lib/ignore/ignore_test.go +++ b/lib/ignore/ignore_test.go @@ -195,6 +195,9 @@ func TestBadPatterns(t *testing.T) { if err == nil { t.Errorf("No error for pattern %q", pat) } + if !IsParseError(err) { + t.Error("Should have been a parse error:", err) + } } } @@ -1006,10 +1009,13 @@ func TestIssue4901(t *testing.T) { for i := 0; i < 2; i++ { err := pats.Load(".stignore") if err == nil { - t.Fatalf("expected an error") + t.Fatal("expected an error") } if fs.IsNotExist(err) { - t.Fatalf("unexpected error type") + t.Fatal("unexpected error type") + } + if !IsParseError(err) { + t.Fatal("failure to load included file should be a parse error") } } @@ -1094,9 +1100,13 @@ func TestPartialIncludeLine(t *testing.T) { } for _, tc := range cases { - if err := pats.Parse(bytes.NewBufferString(tc), ".stignore"); err == nil { + err := pats.Parse(bytes.NewBufferString(tc), ".stignore") + if err == nil { t.Fatal("should error out") } + if !IsParseError(err) { + t.Fatal("failure to load included file should be a parse error") + } } } @@ -1177,5 +1187,8 @@ func TestEmptyPatterns(t *testing.T) { if err == nil { t.Error("Should reject invalid pattern", tc) } + if !IsParseError(err) { + t.Fatal("bad pattern should be a parse error") + } } } diff --git a/lib/model/model.go b/lib/model/model.go index 838f6cd3a..ddde176c6 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -1656,11 +1656,15 @@ func (m *model) GetIgnores(folder string) ([]string, []string, error) { ignores = ignore.New(fs.NewFilesystem(cfg.FilesystemType, cfg.Path)) } - if err := ignores.Load(".stignore"); err != nil && !fs.IsNotExist(err) { - return nil, nil, err + err := ignores.Load(".stignore") + if fs.IsNotExist(err) { + // Having no ignores is not an error. + return nil, nil, nil } - return ignores.Lines(), ignores.Patterns(), nil + // Return lines and patterns, which may have some meaning even when err + // != nil, depending on the specific error. + return ignores.Lines(), ignores.Patterns(), err } func (m *model) SetIgnores(folder string, content []string) error {