search

olson-sean-k / wax

Opinionated and portable globs that can be matched against paths and directory trees.
https://glob.guide
MIT License
112 stars 10 forks source link

Fix glob walking if the provided globs are anchored #56

Open arlyon opened 8 months ago

arlyon commented 8 months ago

I ran into an issue when updating to 0.6.0. For complicated backwards compatibility reasons my project pre-processes globs to always be root-relative and I noticed a bug in the handling of root-relative globs due to the ordering of skipping vs filtering. I have added a test case. All other tests continue to pass.

I had an additional question about the semantics around filtering. As part of our preprocessing we collapse ./ and ../ components so we are not affected however regarding this chunk of code

https://github.com/olson-sean-k/wax/blob/814b5bc7af98c12e6038169c2ce13dbd5c27f05a/src/walk/glob.rs#L358-L361

We filter out all other component types which appears at least to just silently ignore upwards traversals. The answer could be 'these paths are already canonicalized so we don't care' but I think a comment specifically explaining why that is the case would be helpful.

Thanks again for 0.6, the rest of the migration was pretty painless :)

Edit: seems like we have some windows issues. I have a machine laying around will boot it up and figure it out.

arlyon commented 8 months ago

Ok, I added proper handling for windows prefixes. Glob syntax doesn't support prefixes so we just remove them completely in the candidate path before running the program on them.

olson-sean-k commented 7 months ago

Hey, @arlyon! Thanks for the PR and for finding another serious matching bug! 👍🏽

Just some quick thoughts! I'd prefer for CandidatePath to modify its inputs (paths) as little as possible and for matching to handle any necessary transformations. I'm hoping we can leave CandidatePath unchanged for this fix. Regarding tests, I think separate tests for different supported platforms using conditional compilation would be best here. This allows tests to better express platform-specific semantics and test data.

I'll try to review this in more detail and get it landed soon. Apologies for the delayed response! I've been spending most of my available time on a major refactor. 😅