jethrokuan / z

Pure-fish z directory jumping
MIT License
1.26k stars 44 forks source link

Escape special characters #72

Closed krobelus closed 5 years ago

krobelus commented 5 years ago

I understand the data file is sorted by the path name. Not that it matters but this preserves this behavior.

jethrokuan commented 5 years ago

lgtm, but I think it's weird that only _z_add uses perl, while the main z script still uses awk. Preferably one of these dependencies would be removed.

krobelus commented 5 years ago

actually I realized this doesn't fix the real issue, because that one is in __z.fish. I though I had it but it seems like I encountered some spurious issue.. so this change is useless.

Basically z assumes that the user inputs a regex, using tests like $1 ~ q which could be changed to index($1, q) != 0 for fixed string comparison. Let's think about what we want to do here. I think we shouldn't use regex by default. If multiple arguments are given, then all of them have to match, this can easily done without a regex.

jethrokuan commented 5 years ago

Are you referring to #53 ? I recall that the issue was with how awk passed back the path (in single quotes), and has no relation to the use of regex. If you can think of a way to get around it by removing the use of regex, I'm fine with it, but otherwise it's not solving the issue either.

I'm actually not on fish right now, because fish 3.0 uses 100% of my CPU and completely breaks down, but I'll test it properly once you think you have a fix.

krobelus commented 5 years ago

Are you referring to #53 ? I recall that the issue was with how awk passed back the path (in single quotes), and has no relation to the use of regex. If you can think of a way to get around it by removing the use of regex, I'm fine with it, but otherwise it's not solving the issue either.

Yes, I figured out how to fix it. If we pass a string to awk, then backslashes need to be escaped. Additionally strings that are used as regex must be escaped. I also provide tests that fail if the fixes are not applied.

I'm using string escape --style=regex to escape regexes, however that's only available in fish 3.x. I added a workaround for previous versions of fish.

I'm actually not on fish right now, because fish 3.0 uses 100% of my CPU and completely breaks down, but I'll test it properly once you think you have a fix.

Well that's strange. :( Did you try disabling all plugins and configuration? Anyway, I'm happy to step in to fix any issues in this repo.

jethrokuan commented 5 years ago

Yes, I figured out how to fix it. If we pass a string to awk, then backslashes need to be escaped. Additionally strings that are used as regex must be escaped. I also provide tests that fail if the fixes are not applied.

Nice! Thanks for all the effort so far, I can see the considerable amount of work you've put in here.

I'm using string escape --style=regex to escape regexes, however that's only available in fish 3.x. I added a workaround for previous versions of fish.

Cool, the workaround seems relatively simple, although all this string-escaping stuff seems to be adding some complexity. I'd been completely fine if this was only fixed for 3.0 and above

Well that's strange. :( Did you try disabling all plugins and configuration?

Some people are facing the same issue https://github.com/fish-shell/fish-shell/issues/5528, and I don't really use my shell much these days: lots of stuff I used to do I now do in Emacs.

Anyway, I'm happy to step in to fix any issues in this repo.

Happy to add you as co-maintainer if you're up for it. This feels like done software, and there's few issue reports, so it should be relatively low effort.

krobelus commented 5 years ago

Cool, the workaround seems relatively simple, although all this string-escaping stuff seems to be adding some complexity. I'd been completely fine if this was only fixed for 3.0 and above

The support for fish prior to 3.0 adds just that one function (__z_legacy_escape_regex) to replace string escape --regex. I'm pretty sure it's equivalent to the one defined in fish. Considering it's a bug fix, I think we should also support older versions.

Well that's strange. :( Did you try disabling all plugins and configuration?

Some people are facing the same issue fish-shell/fish-shell#5528, and I don't really use my shell much these days:

That looks nasty :(

lots of stuff I used to do I now do in Emacs.

Same here actually but I still heavily depend on my shell.

Anyway, I'm happy to step in to fix any issues in this repo.

Happy to add you as co-maintainer if you're up for it. This feels like done software, and there's few issue reports, so it should be relatively low effort.

Sounds great, thanks!

jethrokuan commented 5 years ago

The support for fish prior to 3.0 adds just that one function (__z_legacy_escape_regex) to replace string escape --regex. I'm pretty sure it's equivalent to the one defined in fish. Considering it's a bug fix, I think we should also support older versions.

yeah I was thinking that if it were any more complicated than that, I'd be fine with it, since it's not particularly deal-breaking, and I try to avoid making the code too complex.

Sounds great, thanks!

Done!