search

mroth / scmpuff

:1234: Numeric file shortcuts for common git commands
https://mroth.github.io/scmpuff/
MIT License
383 stars 22 forks source link

scmpuff expand should escape '*' #44

Closed jdelStrother closed 5 years ago

jdelStrother commented 5 years ago

if a literal '*' is passed to scmpuff expand, it echos it back in the output without any escaping:

$ scmpuff expand -- git log --exclude='refs/wip/*'
git log --exclude=refs/wip/*

Shouldn't this be escaped? How about this PR?

mroth commented 5 years ago

Ah thanks! It looks like the CI error in appveyor is unrelated and is just from ruby changes: https://github.com/bundler/heroku-buildpack-bundler2/pull/1

If you replace the --no-ri and --no-doc in appveyor.yml with --no-document I believe this should probably pass? (Or at least test cleanly in Windows as well).

(Unrelated, but seeing the linked downstream issue, I'd be happy to bundle first-class Fish support into the scmpuff init scripts, and I suspect the Aruba integration tests could be fairly easily updated to test in fish in addition to bash/zsh.)

jdelStrother commented 5 years ago

I'd also like to fix it so that it can handle newlines - eg 

git commit -m "a subject

a body"

Do you think shellEscaper is the way to go to try and cover these cases or do we need something more rigorous? (I confess I'm struggling to figure out what that more rigorous approach might be...)

[EDIT: actually, maybe the newline thing is only an issue in fish. It seems to work as-expected in bash :/ ]

mroth commented 5 years ago

Interesting, I'd never even considered a newline case.

I found an existing package that now exists for shell escaping (https://github.com/alessio/shellescape), however, a.) on a quick glance of it's source code I don't see anything about handling newlines, b.) it appears to be designed for POSIX only, and the more I think about this the more I realize that to be really robust we'd probably need something different/specific for different supported shells.

Another approach might be to try to move the escaping into the shell wrapper scripts, and out of the scmpuff binary, which could simplify shell specific operations to the individual code paths (at the potential cost of more complex shell code). It's also possible there might be an even simpler way to do this, by quoting the arguments in a '' or something -- I'd need to look more into that, its been a while since I've looked at this code.

Since this current fix seems to be working, probably best to open a new issue and we can start discussing the newline thing there, and I'll merge this in the meantime (thanks again for the contribution!).