aureliojargas / txt2regex

Regex wizard for the terminal, written in Bash
https://aurelio.net/projects/txt2regex/
GNU General Public License v2.0
183 stars 26 forks source link

Use quotes around $escape_metachar in escCharList #12

Closed enr0n closed 1 year ago

enr0n commented 1 year ago

The expression uin="${uin/\/$escape_metachar$escape_metachar}" has different output on bash 5.2 then on earlier versions. Namely, $escape_metachar is not treated as a string literal, so the escape characters are applied which results in half as many escape chars in the output:

$ bash --version | head -1 GNU bash, version 5.2.0(1)-rc2 (x86_64-pc-linux-gnu) $ uin='[]'; escape_metachar='\'; echo ${uin/\/$escape_metachar$escape_metachar} [\]

vs.

$ bash --version | head -1 GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu) $ uin='[]'; escape_metachar='\'; echo ${uin/\/$escape_metachar$escape_metachar} [\\]

To fix this, add quotes around $escape_metachar in this substitution, which works on bash 5.2 as well as earlier versions:

$ bash --version | head -1 GNU bash, version 5.2.0(1)-rc2 (x86_64-pc-linux-gnu) $ uin='[]'; escape_metachar='\'; echo ${uin/\/"$escape_metachar$escape_metachar"} [\\]

and

$ bash --version | head -1 GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu) $ uin='[]'; escape_metachar='\'; echo ${uin/\/"$escape_metachar$escape_metachar"} [\\]

aureliojargas commented 1 year ago

Thanks @enr0n for reporting on this issue and even providing a patch 🏅

I've run the tests on your code (make test-bash), but unfortunately the fix does not work for Bash version 4.2 and earlier 😞

For those versions, the extra quotes appear in the output:

$ make test-bash
...
Testing in Bash version 4.2
Testing file tests/cmdline.md
Testing file tests/features.md
--------------------------------------------------
[FAILED #52, line 77] txt2regex --all --history '241¤\'
@@ -1,23 +1,23 @@
- Regex awk       : [\\]
- Regex chicken   : [\\\\]
+ Regex awk       : ["\\"]
+ Regex chicken   : ["\\\\"]
  Regex ed        : [\]
...

Today I have also added some commits to the master branch to fix the CI here, so the GitHub Actions will run on new pull requests, executing the whole test suite on them (making this kind of issue visible). Rebasing this PR will also trigger the CI.

enr0n commented 1 year ago

It may not be the cleanest solution, but I added a check on the bash version and only use the quotes on bash 5.2 and newer.

aureliojargas commented 1 year ago

The CI is green. Thank you very much for your time and patience @enr0n ❤️