From 4dc476f286814ce7bacc31a5b6540601c72c1364 Mon Sep 17 00:00:00 2001
From: "brian m. carlson" <bk2204@github.com>
Date: Thu, 25 Jun 2020 18:59:46 +0000
Subject: [PATCH] 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.
---
 filepathfilter/filepathfilter.go | 41 +++++++++++++++++++++++++++-----
 locking/lockable.go              |  2 +-
 t/t-unlock.sh                    | 40 +++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 7 deletions(-)

diff --git a/filepathfilter/filepathfilter.go b/filepathfilter/filepathfilter.go
index a34c545c..72103af7 100644
--- a/filepathfilter/filepathfilter.go
+++ b/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,6 +85,15 @@ 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())
@@ -73,6 +101,7 @@ func (f *Filter) Allows(filename string) bool {
 		}
 	}
 
+	// No patterns matched and our default value is true.
 	tracerx.Printf("filepathfilter: accepting %q", filename)
 	return true
 }
diff --git a/locking/lockable.go b/locking/lockable.go
index 12602ffc..b81909c4 100644
--- a/locking/lockable.go
+++ b/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,
diff --git a/t/t-unlock.sh b/t/t-unlock.sh
index 6bbab2d2..0981e33c 100755
--- a/t/t-unlock.sh
+++ b/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