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

Allow skipping validation when Matching #92

Closed
wingsofovnia opened this issue Apr 4, 2024 · 2 comments
Closed

Allow skipping validation when Matching #92

wingsofovnia opened this issue Apr 4, 2024 · 2 comments

Comments

@wingsofovnia
Copy link

doMatchWithSeparator does support turning off validation

func doMatchWithSeparator(pattern, name string, separator rune, validate bool, doublestarPatternBacktrack, doublestarNameBacktrack, starPatternBacktrack, starNameBacktrack, patIdx, nameIdx int) (matched bool, err error) {

However, it's public counter part doesn't and always enables validation:

doublestar/match.go

Lines 52 to 53 in 180028b

func Match(pattern, name string) (bool, error) {
return matchWithSeparator(pattern, name, '/', true)

Allowing matching without validation can save some cycles in the use cases where glob patterns have been already validated at an earlier point. A suggested (public) func may look like:

func MatchUnvalidated(pattern, name string) (bool, error) {
	return matchWithSeparator(pattern, name, '/', false)
}
@jbedard
Copy link

jbedard commented Oct 10, 2024

I've been having the same performance concerns for a while, also from a bazel gazelle extension where the same globs are invoked a significant number of times.

@bmatcuk
Copy link
Owner

bmatcuk commented Oct 11, 2024

Sorry I haven't replied to this in so long - life is busy 😢

So, the validate flag to doMatchWithSeparator only affects one case: when matching reaches the end of name before reaching the end of pattern, ie, in something like Match("a/b/c", "a").

doublestar/match.go

Lines 286 to 288 in 424062b

if validate && patIdx < patLen && !doValidatePattern(pattern[patIdx:], separator) {
return false, ErrBadPattern
}

That's the only case Match runs validation. So, I wouldn't expect a huge performance improvement from a MatchUnvalidated. But, I suppose if matching is run many, many times, it would add up.

I cut a v4.7.0 which includes a MatchUnvalidated. It only returns a bool instead of bool, error.

fmeum pushed a commit to bazel-contrib/bazel-gazelle that referenced this issue Oct 21, 2024
doublestar now has an option to match without re-validating every time,
see bmatcuk/doublestar#92

**What type of PR is this?**

> Other

**What package or component does this PR mostly affect?**

> all

**What does this PR do? Why is it needed?**

Performance
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 23, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
jbedard added a commit to aspect-build/aspect-cli that referenced this issue Oct 24, 2024
See bmatcuk/doublestar#92 - each time a glob
is executed it gets validated if it does not successfully match a path.
This now validates it once up front and then avoids re-validating on
every (non-matched) glob invocation.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

`aspect configure` glob patterns are now validated once up front instead
of per evaluation.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 82135c3f6342ea0893d5701b6bb4cfc451e6fe0b
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

No branches or pull requests

3 participants