Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #88 Globstar matches zero directories #89

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

mmxmb
Copy link
Contributor

@mmxmb mmxmb commented Oct 18, 2023

Addressing the issue raised in #88

Summary of the issue

In the current implementation, doublestar.Match("a/**/", "a") and doublestar.Match("a/**/", "a/") both return false.

Bash shopt docs say that ** followed by a / will match zero or more directories and subdirectories.

globstar
If set, the pattern ‘**’ used in a filename expansion context will match all files and zero or more directories and subdirectories. If the pattern is followed by a ‘/’, only directories and subdirectories match.

Bash 5.2 implementation confirms this behavior:

bash-5.2$ bash --version
GNU bash, version 5.2.15(1)-release (aarch64-apple-darwin23.0.0)
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
bash-5.2$ mkdir a
bash-5.2$ shopt -s globstar
bash-5.2$ ls -d a/**/
a/

What changed?

Changed isZeroLengthPattern in match.go to recognize **/ and /**/ as zero length patterns, in addition to /**, *, and **.

How was this tested?

Added two new test cases to matchTests in doublestar_test.go:

  • {"a/**/", "a", true, true, nil, false, false, false, false, 4, 4}. This test case verifies that /**/ is a zero length pattern.
  • {"a/**/", "a/", true, true, nil, false, false, false, false, 4, 4} This test case verifies that **/ is a zero length pattern. I was less certain whether **/ should be considered a zero length pattern; but it's the only way I came up with to make this test case pass. And it felt wrong to omit this test case, since there's already a very similar test case with a successful match {"a/**", "a/", true, true, nil, false, false, false, false, 7, 7}

I wanted to also run these tests on disk but the current implementation of FilepathGlob in utils.go makes it impossible and changing its implementation is outside of the scope of this change. The issue there is on this line:

pattern = filepath.Clean(pattern)

This line removes the trailing / from a pattern like a/**/ which changes its meaning. a/**/ matches zero or more directories and subdirectories at path a/. a/** matches all files, and zero or more directories and subdirectories at path a/.

Luckily, with GlobWalk I can add an additional filtering function for each match so that I can ensure that the pattern that I use matches only directories and not regular files.

@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (465a339) 85.42% compared to head (09b9aee) 85.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   85.42%   85.50%   +0.07%     
==========================================
  Files           5        5              
  Lines         940      945       +5     
==========================================
+ Hits          803      808       +5     
  Misses        106      106              
  Partials       31       31              
Files Coverage Δ
match.go 90.00% <100.00%> (+0.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bmatcuk bmatcuk changed the title Globstar matches zero directories fixes #88 Globstar matches zero directories Oct 21, 2023
@bmatcuk bmatcuk merged commit 5df0d9d into bmatcuk:master Oct 21, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants