locking: don't assume files are lockable if no patterns exist

Whenever we use a file path filter and there are no included patterns,
we default to assuming the files match.  For many cases, this is fine,
but this is not useful for checking whether a file is lockable: if no
files are listed as lockable, then no files are lockable.

Let's add an option for our filters to specify the default value if no
patterns match.  If that default is false, like it should be for
locking, then skip traversing the exclude patterns altogether, since
anything that's not included cannot change the result.  We test both the
case that lockable patterns are present and absent in our test.

Author: bk2204

filepathfilter/filepathfilter.go

@@ -16,18 +16,37 @@ type Pattern interface {
 }
 
 type Filter struct {
-	include []Pattern
-	exclude []Pattern
+	include      []Pattern
+	exclude      []Pattern
+	defaultValue bool
 }
 
-func NewFromPatterns(include, exclude []Pattern) *Filter {
-	return &Filter{include: include, exclude: exclude}
+type options struct {
+	defaultValue bool
 }
 
-func New(include, exclude []string) *Filter {
+type option func(*options)
+
+// DefaultValue is an option representing the default value of a filepathfilter
+// if no patterns match.  If this option is not provided, the default is true.
+func DefaultValue(val bool) option {
+	return func(args *options) {
+		args.defaultValue = val
+	}
+}
+
+func NewFromPatterns(include, exclude []Pattern, setters ...option) *Filter {
+	args := &options{defaultValue: true}
+	for _, setter := range setters {
+		setter(args)
+	}
+	return &Filter{include: include, exclude: exclude, defaultValue: args.defaultValue}
+}
+
+func New(include, exclude []string, setters ...option) *Filter {
 	return NewFromPatterns(
 		convertToWildmatch(include),
-		convertToWildmatch(exclude))
+		convertToWildmatch(exclude), setters...)
 }
 
 // Include returns the result of calling String() on each Pattern in the
@@ -66,13 +85,23 @@ func (f *Filter) Allows(filename string) bool {
 		return false
 	}
 
+	// Beyond this point, the only values we can logically return are false
+	// or the default value.  If the default is false, then there's no point
+	// traversing the exclude patterns because the return value will always
+	// be false; we'd do extra work for no functional benefit.
+	if !included && !f.defaultValue {
+		tracerx.Printf("filepathfilter: rejecting %q", filename)
+		return false
+	}
+
 	for _, ex := range f.exclude {
 		if ex.Match(filename) {
 			tracerx.Printf("filepathfilter: rejecting %q via %q", filename, ex.String())
 			return false
 		}
 	}
 
+	// No patterns matched and our default value is true.
 	tracerx.Printf("filepathfilter: accepting %q", filename)
 	return true
 }

locking/lockable.go

@@ -47,7 +47,7 @@ func (c *Client) refreshLockablePatterns() {
 			c.lockablePatterns = append(c.lockablePatterns, filepath.ToSlash(p.Path))
 		}
 	}
-	c.lockableFilter = filepathfilter.New(c.lockablePatterns, nil)
+	c.lockableFilter = filepathfilter.New(c.lockablePatterns, nil, filepathfilter.DefaultValue(false))
 }
 
 // IsFileLockable returns whether a specific file path is marked as Lockable,

t/t-unlock.sh

@@ -185,6 +185,46 @@ begin_test "unlocking nonexistent file"
 )
 end_test
 
+begin_test "unlocking unlockable file"
+(
+  set -e
+
+  reponame="unlock-unlockable-file"
+  # Try with lockable patterns.
+  setup_repo "$reponame" "a.dat"
+
+  touch README.md
+  git add README.md
+  git commit -m 'Add README'
+
+  git lfs lock --json "README.md" | tee lock.log
+  id=$(assert_lock lock.log README.md)
+  assert_server_lock "$reponame" "$id"
+  assert_file_writeable "README.md"
+
+  git lfs unlock --force "README.md" 2>&1 | tee unlock.log
+  refute_server_lock "$reponame" "$id"
+  assert_file_writeable "README.md"
+
+  cd "$TRASHDIR"
+  # Try without any lockable patterns.
+  setup_remote_repo_with_file "$reponame-2" "a.dat"
+
+  touch README.md
+  git add README.md
+  git commit -m 'Add README'
+
+  git lfs lock --json "README.md" | tee lock.log
+  id=$(assert_lock lock.log README.md)
+  assert_server_lock "$reponame-2" "$id"
+  assert_file_writeable "README.md"
+
+  git lfs unlock --force "README.md" 2>&1 | tee unlock.log
+  refute_server_lock "$reponame-2" "$id"
+  assert_file_writeable "README.md"
+)
+end_test
+
 begin_test "unlocking a lock (--json)"
 (
   set -e