mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.35k stars 346 forks source link

expand: fix sequential '\' in double quote literals #1107

Closed cfogrady closed 3 weeks ago

cfogrady commented 3 weeks ago

Fix for https://github.com/mvdan/sh/issues/1106

mvdan commented 3 weeks ago

Thanks! I actually avoid unit tests where possible, and instead test the exposed API. In this case I would add integration tests in the interpreter, for example adding to these around line 400 in interp_test.go:

    // escaped chars
    {"echo a\\b", "ab\n"},
    {"echo a\\ b", "a b\n"},
    {"echo \\$a", "$a\n"},
    {"echo \"a\\b\"", "a\\b\n"},
    {"echo 'a\\b'", "a\\b\n"},
    {"echo \"a\\\nb\"", "ab\n"},
    {"echo 'a\\\nb'", "a\\\nb\n"},
    {`echo "\""`, "\"\n"},
    {`echo \\`, "\\\n"},
    {`echo \\\\`, "\\\\\n"},
    {`echo \`, "\n"},

I know that means there isn't test coverage in the same package, but I think proper coverage across the module is still fine. And, ultimately, most of these bugs in the expand package only matter for users of the interp package.

mvdan commented 3 weeks ago

And avoiding unit tests also means that you don't need to add one more function to test.

cfogrady commented 3 weeks ago

And avoiding unit tests also means that you don't need to add one more function to test.

For clarity, do you prefer to keep the logic inlined? My default is to split out what I would consider to be tangential logic because I think it helps readability, but you're the maintainer.

mvdan commented 3 weeks ago

Yes, please keep the code where it was - the func isn't particularly large or very indented, so I don't think it requires splitting up just yet.

cfogrady commented 3 weeks ago

Function is inlined once again, and the (in my opinion) essential test cases have been added to the interp_test.go test cases.

mvdan commented 3 weeks ago

There was a minor gofmt issue which I fixed. I will squash-merge, as I prefer to keep the git history tidy. For future PRs, feel free to squash into a single commit yourself with a good commit message, and I'll merge as-is.

cfogrady commented 3 weeks ago

Sounds good!

mvdan commented 3 weeks ago

(as usual got sidetracked waiting for CI to pass, merging now)