Skip to content
New issue

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

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

Already on GitHub? # to your account

Draft fix for duplicate leading double-asterisk, and **/ edge cases #45

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

JonjonHays
Copy link

Hi there @cpburnz! First off, thanks so much for this library -- it's been incredibly useful to my team! I found a couple edge-case-y bugs. There's two issues I found, and the test cases I added are mostly self documenting as far as describing the bugs. I've added a (possibly hacky) fix for the first one, "duplicate leading double-asterisk". Basically the change I made just trims duplicate **/s ...The second one is for the **/ pattern. This one will likely require changes in the _translate_segment_glob(seg) method (I'm not really sure, actually). Anyways, I think we will probably deal with these bugs within our own code, but I figured I'd at least provide a couple test cases and a launch point instead of just filing an issue. Thanks again for the awesome tool, and please ping me if you decide to go after this. Cheers!

...also, just for a little more context, both issues produce patterns that impose a leading slash, or a double slash somewhere in the path. Here's an example produced from **/** as the raw pattern: https://regex101.com/r/KIDJtA/1

@cpburnz
Copy link
Owner

cpburnz commented Jun 3, 2021

Hi @JonjonHays, thanks for the pull request.

For the first bug, there's a slight issue with it. It strips out all non-leading **s. E.g., **/a/**/b/** with segments ['**', 'a', '**', 'b', '**'] gets converted to ['**', 'a', 'b']. All that's needed to fix it is to add a or trimmed_segs[-1] != '**' to the if statement. I'll fix this. No need to modify the pull request.

For the second bug, that's an interesting case. Both the patterns ** and **/** have the regex '^.+$' yet **/ has '^(?:.+/)?/.*$' instead. I have an idea of how to handle this which I'll experiment with.

@cpburnz cpburnz merged commit 74b9ea5 into cpburnz:master Jun 3, 2021
@cpburnz
Copy link
Owner

cpburnz commented Jun 12, 2021

@JonjonHays I have a fix for the **/ pattern. You're test case was correct.

@JonjonHays
Copy link
Author

Thanks for the update/merge @cpburnz ! Much appreciated =)

bors bot referenced this pull request in rehandalal/therapist Jul 25, 2021
180: Bump pathspec from 0.8.1 to 0.9.0 r=rehandalal a=dependabot[bot]

Bumps [pathspec](https://github.com/cpburnz/python-path-specification) from 0.8.1 to 0.9.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/cpburnz/python-path-specification/releases">pathspec's releases</a>.</em></p>
<blockquote>
<h2>v0.9.0</h2>
<p>Release v0.9.0.</p>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/cpburnz/python-path-specification/blob/master/CHANGES.rst">pathspec's changelog</a>.</em></p>
<blockquote>
<h2>0.9.0 (2021-07-17)</h2>
<ul>
<li><code>Issue [#44](https://github.com/cpburnz/python-path-specification/issues/44)</code><em>/<code>Issue [#50](https://github.com/cpburnz/python-path-specification/issues/50)</code></em>: Raise <code>GitWildMatchPatternError</code> for invalid git patterns.</li>
<li><code>Issue [#45](https://github.com/cpburnz/python-path-specification/issues/45)</code>_: Fix for duplicate leading double-asterisk, and edge cases.</li>
<li><code>Issue [#46](https://github.com/cpburnz/python-path-specification/issues/46)</code>_: Fix matching absolute paths.</li>
<li>API change: <code>util.normalize_files()</code> now returns a <code>Dict[str, List[pathlike]]</code> instead of a <code>Dict[str, pathlike]</code>.</li>
<li>Added type hinting.</li>
</ul>
<p>.. _<code>Issue [#44](https://github.com/cpburnz/python-path-specification/issues/44)</code>: <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/issues/44">cpburnz/python-path-specification#44</a>
.. _<code>Issue [#45](https://github.com/cpburnz/python-path-specification/issues/45)</code>: <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/pull/45">cpburnz/python-path-specification#45</a>
.. _<code>Issue [#46](https://github.com/cpburnz/python-path-specification/issues/46)</code>: <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/issues/46">cpburnz/python-path-specification#46</a>
.. _<code>Issue [#50](https://github.com/cpburnz/python-path-specification/issues/50)</code>: <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/pull/50">cpburnz/python-path-specification#50</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/7ca27e2300ced6364a3816820eb2532c89c7d01c"><code>7ca27e2</code></a> Release v0.9.0</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/816aba44fd44a42dd4b65d04f182d60c580a7348"><code>816aba4</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/issues/50">#50</a> from SebastiaanZ/bug/44-index-error</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/ed4481f2d2708d47a25e828fd5be8980fbcfe1f4"><code>ed4481f</code></a> Address <a href="https://github-redirect.dependabot.com/cpburnz/python-path-specification/issues/44">#44</a>: raise informative exception for invalid git patterns</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/c00b332b2075548ee0c0673b72d7f2570d12ffe6"><code>c00b332</code></a> Add type hints</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/19daf50ec0b49c5ec7f20ac95ed6d7f5922c4a5d"><code>19daf50</code></a> Update credits</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/4d6c0bd1f53836f2fc59e6e5a1f3555558d82133"><code>4d6c0bd</code></a> Fix matching absolute paths</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/0761facce8d3f960574ef3ae5636243fe2f907e9"><code>0761fac</code></a> Update CHANGES</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/08ba4e62c6476a0f4ff4c798b2ec16943e8ab85f"><code>08ba4e6</code></a> Support '**/'</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/789fa6ff8998abf5fae20f14793139b6fd94f303"><code>789fa6f</code></a> Expand some tests</li>
<li><a href="https://github.com/cpburnz/python-path-specification/commit/a6ebcba01c0d11377cd432640504e7a631b43ceb"><code>a6ebcba</code></a> Handle all duplicate double-asterisk sequences</li>
<li>Additional commits viewable in <a href="https://github.com/cpburnz/python-path-specification/compare/v0.8.1...v0.9.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pathspec&package-manager=pip&previous-version=0.8.1&new-version=0.9.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- ``@dependabot` rebase` will rebase this PR
- ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it
- ``@dependabot` merge` will merge this PR after your CI passes on it
- ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it
- ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging
- ``@dependabot` reopen` will reopen this PR if it is closed
- ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants