search

beyondgrep / ack2

**ack 2 is no longer being maintained. ack 3 is the latest version.**
https://github.com/beyondgrep/ack3/
Other
1.48k stars 138 forks source link

Strip quotes from color options; fixes #416 #441

Closed jgbishop closed 10 years ago

jgbishop commented 10 years ago

I believe the patch below fixes this issue (it does on my Windows system). I created a new subroutine that removes any single or double quotes from the given string, and then use it to modify the appropriate color options. In my test sandbox, this makes Term::ANSIColor::colored happy.

If there's a cleaner, or more succinct way of doing this, let me know. I just wanted to contribute to this great project (it's helping me learn git and improving my Perl at the same time).

hoelzro commented 10 years ago

Hi @jgbishop! Thanks for looking at this issue, and for taking the time to write code to fix it. I'm not too familiar with how environment variables work on Windows, but how are you setting them? I'm wondering if there's a way to set them without bringing the quotes in. Then again, if many people are making this mistake, this might be a good patch to have in ack to help users out.

jgbishop commented 10 years ago

Maybe I'm misunderstanding you, but none of the values are explicitly set in an environment variable. From the original bug report, the issue at hand crops up from using the --color-match option in an .ackrc file (using this flag directly from the command line works just fine). The issue is that those color options can take a space-separated list. When passing such a list from the command line, the quotes used to surround the list get eaten for you by the shell. Specifying a similar list in the .ackrc file prevents that; the shell was never at play, so the quotes stick around.

jgbishop commented 10 years ago

The second commit here can be ignored. I was updating my local repo with the changes from the master. My git-fu is weak enough to not know how to prevent that merge from showing up in this pull-request...

hoelzro commented 10 years ago

@jgbishop I don't know if stripping quotes is the right thing to do here; maybe we should just be telling people that quotes aren't needed in config files.

jgbishop commented 10 years ago

Makes sense to me. I'm working on fixing some low-hanging documentation issues, and I'll try to find a place to stick that information.