Replace fnmatch with wildmatch #5110
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Fixes #5093 |
e0ff9d9 to
e6785ed
Compare
|
Rebased to fix conflicts with #5097
|
|
Fixes #5068 |
|
Would you mind adding a paragraph to |
In commit 70a8fc999d (stop using fnmatch (either native or compat), 2014-02-15), upstream git has switched over all code from their internal fnmatch copy to its new wildmatch code. We haven't followed suit, and thus have developed some incompatibilities in how we match regular expressions. Import git's wildmatch from v2.22.0 and add a test suite based on their t3070-wildmatch.sh tests.
We're about to phase out our bundled fnmatch implementation as git.git has moved to wildmatch long ago in 2014. To make it easier to spot which files are stilll using fnmatch, remove the implicit "fnmatch.h" include in "posix.h" and instead include it explicitly.
Upstream git.git has converted its codebase to use wildcard in favor of fnmatch in commit 70a8fc999d (stop using fnmatch (either native or compat), 2014-02-15). To keep our own regex-matching in line with what git does, convert all trivial instances of `fnmatch` usage to use `wildcard`, instead. Trivial usage is defined to be use of `fnmatch` with either no flags or flags that have a 1:1 equivalent in wildmatch (PATHNAME, IGNORECASE).
The function `do_match_gitdir` has some horribly named parameters and variables. Rename them to improve readability. Furthermore, fix a potentially undetected out-of-memory condition when appending "**" to the pattern.
When evaluating "gitdir:" and "gitdir/i:" conditionals, we currently compare the given pattern with the value of `git_repository_path`. Thing is though that `git_repository_path` returns the gitdir path with trailing '/', while we actually need to match against the gitdir without it. Fix this issue by stripping the trailing '/' previous to matching. Add various tests to ensure we get this right.
We currently use `p_fnmatch` to compute whether a given "gitdir:" or "gitdir/i:" conditional matches the current configuration file path. As git.git has moved to use `wildmatch` instead of `p_fnmatch` throughout its complete codebase, we evaluate conditionals inconsistently with git.git in some special cases. Convert `p_fnmatch` to use `wildmatch`. The `FNM_LEADINGDIR` flag cannot be translated to `wildmatch`, but in fact git.git doesn't use it here either. And in fact, dropping it while we go increases compatibility with git.git.
Upstream git has converted to use `wildmatch` instead of `fnmatch`. Convert our gitattributes logic to use `wildmatch` as the last user of `fnmatch`. Please, don't expect I know what I'm doing here: the fnmatch parser is one of the most fun things to play around with as it has a sh*tload of weird cases. In all honesty, I'm simply relying on our tests that are by now rather comprehensive in that area. The conversion actually fixes compatibility with how git.git parser "**" patterns when the given path does not contain any directory separators. Previously, a pattern "**.foo" erroneously wouldn't match a file "x.foo", while git.git would match. Remove the new-unused LEADINGDIR/NOLEADINGDIR flags for `git_attr_fnmatch`.
The `fnmatch` code has now been completely replaced by `wildmatch`, same as upstream git.git has been doing in 2014. Remove it.
Done! |
|
Nice one, thanks for this @pks-t! |
ethomson
approved these changes
Jun 15, 2019
nathansobo
pushed a commit
to atom/git-utils
that referenced
this pull request
Jun 17, 2019
nathansobo
pushed a commit
to atom/git-utils
that referenced
this pull request
Jun 17, 2019
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upstream git.git has replaced fnmatch with wildmatch back in commit 70a8fc999d (stop using fnmatch (either native or compat), 2014-02-15). wildmatch has different semantics than fnmatch has and thus may show slightly different behaviour than fnmatch. In fact, by replacing fnmatch with wildmatch, we fix compatibility with git.git in at least two places:
There's probably more compatibility improvements as fnmatch/wildmatch are used at various locations throught the tree, but only those two struck me while working on the replacement.
Anyway. Getting this merged will have to wait until @ethomson has determined whether we're even allowed to use "wildmatch.c" from git.git, depending on its license history.