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

Escape special characters #72

Merged
merged 6 commits into from Jan 29, 2019
Merged

Escape special characters #72

merged 6 commits into from Jan 29, 2019

Conversation

krobelus
Copy link
Collaborator

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

@jethrokuan
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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 krobelus changed the title Rewrite __z_add in perl for proper escaping Escape special characters Jan 27, 2019
awk treats variables like string literals; so a backslash will be
interpreted as the start of an escape sequence. Escape backslashes
to avoid this.
@krobelus
Copy link
Collaborator Author

krobelus commented Jan 27, 2019

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.

The query is used as regex, so special pattern characters need to be
escaped; fish builtin "string escape --style=regex" seems to work
perfectly.

Also escape backslashes to avoid interpretation as escape sequences.
@jethrokuan
Copy link
Owner

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 fish-shell/fish-shell#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.

functions/__z.fish Outdated Show resolved Hide resolved
@krobelus
Copy link
Collaborator Author

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!

Co-Authored-By: krobelus <aclopte@gmail.com>
@jethrokuan
Copy link
Owner

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!

@jethrokuan jethrokuan merged commit d978d54 into jethrokuan:master Jan 29, 2019
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

Successfully merging this pull request may close these issues.

None yet

2 participants