Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

gps: pkgtree: fix and simplify CreateIgnorePrefixTree #1266

Closed
wants to merge 1 commit into from
Closed

gps: pkgtree: fix and simplify CreateIgnorePrefixTree #1266

wants to merge 1 commit into from

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Oct 13, 2017

What does this do / why do we need it?

This PR fixes a small inefficiency bug in pkgtree.CreateIgnorePrefixTree, and further optimizes and simplifies some of the surrounding code. Discovered while investigating #1264, but not expected to resolve it.


// Create a sorted list of all the ignores to have a proper order in
// ignores parsing.
sortedIgnores := make([]string, len(ig))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intention here was make([]string, 0, len(ig)), but I think that optimization no longer makes sense after moving the filtering up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug here was that making with the length only initialized sortedIgnores with len(ig) empty strings, which made append grow the slice anyways (up to 2*len(ig)), slowing down the sort and second loop, but the extra empty entries would end up being filtered out.

if i == "*" {
continue
// Skip global and non-recursive ignore.
if len(k) > 1 && k[len(k)-1] == '*' {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering on the first loop reduces the slice length for sorting and the second loop, and enables an earlier exit path.

@sdboyer
Copy link
Member

sdboyer commented Oct 13, 2017

i've got a PR coming up that removes this func entirely in favor of a new approach, so we can not worry about these bits 😄

@sdboyer sdboyer closed this Oct 13, 2017
@jmank88 jmank88 deleted the ignore_pre branch October 13, 2017 13:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants