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

scmpuff expand should escape '*' #44

Merged
merged 2 commits into from
May 23, 2019
Merged

Conversation

jdelStrother
Copy link
Contributor

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

mroth commented May 22, 2019

Ah thanks! It looks like the CI error in appveyor is unrelated and is just from ruby changes:
rubygems/heroku-buildpack-bundler2#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
Copy link
Contributor Author

jdelStrother commented May 23, 2019

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

mroth commented May 23, 2019

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!).

@mroth mroth merged commit 5688434 into mroth:master May 23, 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.

2 participants