lib/ignore: Add directory separator to glob.Compile call

[imsodin]

The current implementation of gobwas/glob in lib/ignore does not account for directory separators ('/'). Thus the distinction between '' and '**' is meaningless. Currently both the patterns 'ab' and 'a**b' match the path 'a/b'.

I confirmed the current behavior looking at the last file received entry in the GUI. But as I am only familiar with syncthing-inotify and it uses lib/ignore for its filtering as well, I tested this PR with debug information from syncthing-inotify. The test setup is trivial, just put something like the strings mentioned into .stignore.

[calmh]

@st-jenkins add to white list

[calmh]

LGTM but could we get a unit test for this as well to catch regressions in the future? Somewhere in lib/ignore/ignore_test.go should be appropriate.

Also to ensure that this does the right thing on Windows as well (it should, but testing will verify it).

[imsodin]

Would this test do?

[AudriusButkevicius]

I wonder if the pattern separators are normalised somewhere along the way, so that this gets fixed on windows.

[imsodin]

@AudriusButkevicius By normalised do you mean converted to slashs? If yes, this happens here.

[AudriusButkevicius]

@st-review lgtm

[st-review]

@AudriusButkevicius: Noted! Need another LGTM or explicit merge command.

[calmh]

How about this:

func TestIssue3674(t *testing.T) {
    stignore := `
    a*b
    a**c
    `

    testcases := []struct {
        file    string
        matches bool
    }{
        {"ab", true},
        {"asdfb", true},
        {"ac", true},
        {"asdfc", true},
        {"as/db", false},
        {"as/dc", true},
    }

    pats := New(true)
    err := pats.Parse(bytes.NewBufferString(stignore), ".stignore")
    if err != nil {
        t.Fatal(err)
    }

    for _, tc := range testcases {
        res := pats.Match(tc.file).IsIgnored()
        if res != tc.matches {
            t.Errorf("Matches(%q) == %v, expected %v", tc.file, res, tc.matches)
        }
    }
}

Which fails now, and should hopefully pass with your patch:

jb@unu:~/s/g/s/s/l/ignore $ go test -v -run TestIssue3674
=== RUN   TestIssue3674
--- FAIL: TestIssue3674 (0.00s)
    ignore_test.go:768: Matches("as/db") == true, expected false
FAIL
FAIL    github.com/syncthing/syncthing/lib/ignore   0.006s

[imsodin]

Changed it to calmh's test function and yes, it does pass.

[calmh]

👍

[calmh]

@st-jenkins retest this please

[calmh]

@st-review lgtm

[st-review]

@calmh: Noted! Need another LGTM or explicit merge command.

[calmh]

wat

[calmh]

@st-review merge it then

[st-review]

@calmh: Build status is pending. I'll wait until it goes green and then merge!

[st-review]

👌 Merged as 7c37301. Thanks, @imsodin!