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...

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) {
+            if (!$scope.system || !$scope.config || !$scope.config.gui) {
-                // We need both to be able to determine the state.
+                // 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..."));
+            $scope.ignores.text = 'Loading...';
-            $('#folder-ignores textarea').attr('disabled', 'disabled');
+            $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'));
+                    $scope.ignores.text = $scope.currentFolder.ignores.join('\n');
-                    $('#folder-ignores textarea').removeAttr('disabled');
+                    $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("");
+                $scope.ignores.text = '';
-                $('#folder-ignores textarea').removeAttr('disabled');
+                $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("");
+            $scope.ignores.text = '';
-            $('#folder-ignores textarea').removeAttr('disabled');
+            $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 ignoresLoaded = !$scope.ignores.disabled;
-            var ignores = $('#folder-ignores textarea').val().split('\n');
+            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 = [];

gui/default/syncthing/folder/editFolderModalView.html

@@ -9,6 +9,7 @@
         <li><a data-toggle="tab" href="#folder-advanced"><span class="fas fa-cogs"></span> <span translate>Advanced</span></a></li>
       </ul>
       <div class="tab-content">
+
         <div id="folder-general" class="tab-pane in active">
           <div class="form-group" ng-class="{'has-error': folderEditor.folderLabel.$invalid && folderEditor.folderLabel.$dirty}">
             <label for="folderLabel"><span translate>Folder Label</span></label>
@@ -43,6 +44,7 @@
             </p>
           </div>
         </div>
+
         <div id="folder-sharing" class="tab-pane">
           <div class="form-group" ng-if="currentFolder.sharedDevices.length">
             <label translate>Currently Shared With Devices</label>
@@ -82,6 +84,7 @@
             </div>
           </div>
         </div>
+
         <div id="folder-versioning" class="tab-pane">
           <div class="form-group">
             <label translate>File Versioning</label>&emsp;<a href="https://docs.syncthing.net/users/versioning.html" target="_blank"><span class="fas fa-question-circle"></span>&nbsp;<span translate>Help</span></a>
@@ -142,9 +145,15 @@
             </p>
           </div>
         </div>
+
         <div id="folder-ignores" class="tab-pane">
           <p translate>Enter ignore patterns, one per line.</p>
-          <textarea class="form-control" rows="5"></textarea>
+          <div ng-class="{'has-error': ignores.error != null}">
+            <textarea class="form-control" rows="5" ng-model="ignores.text" ng-disabled="ignores.disabled"></textarea>
+            <p class="help-block" ng-if="ignores.error">
+              {{ignores.error}}
+            </p>
+          </div>
           <hr />
           <p class="small"><span translate>Quick guide to supported patterns</span> (<a href="https://docs.syncthing.net/users/ignoring.html" target="_blank" translate>full documentation</a>):</p>
           <dl class="dl-horizontal dl-narrow small">
@@ -165,6 +174,7 @@
           <span translate ng-show="editingExisting" translate-value-path="{{currentFolder.path}}{{system.pathSeparator}}.stignore">Editing {%path%}.</span>
           <span translate ng-show="!editingExisting" translate-value-path="{{currentFolder.path}}{{system.pathSeparator}}.stignore">Creating ignore patterns, overwriting an existing file at {%path%}.</span>
         </div>
+
         <div id="folder-advanced" class="tab-pane">
           <div class="row form-group" ng-class="{'has-error': folderEditor.rescanIntervalS.$invalid && folderEditor.rescanIntervalS.$dirty}">
             <div class="col-md-12">

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)
+	lines, patterns, err := s.model.GetIgnores(folder)
-	if err != nil {
+	if err != nil && !ignore.IsParseError(err) {
 		http.Error(w, err.Error(), 500)
 		return
 	}
 
-	sendJSON(w, map[string][]string{
+	sendJSON(w, map[string]interface{}{
-		"ignore":   ignores,
+		"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
+}

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
 		}
 	}
 

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")
+		}
 	}
 }

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) {
+	err := ignores.Load(".stignore")
-		return nil, nil, err
+	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 {