-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: $EDITOR argument parsing #1595
Conversation
4acd97c
to
5293f8a
Compare
5293f8a
to
fd4c5db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very minor quibble with the usage of unwrap(), but it otherwise looks good to me! Once that's fixed this should be good to merge.
The fact that @jRimbault had to intuit the use of vim --someoption
in #1591 has me a little nervous. After this is merged, I think we should add some actual tests to make sure those kinds of things are being handled properly.
I didn't exactly "intuit" the use case of There is still at least one problematic case I can see: if the editor part in the environment variable contains spaces and is not quoted ( Edit: as a sidenote in my quick cursory looking through the code I noticed a lot of duplicated code, and a few |
Yes, but I think this becomes unreliable as the history grows longer. I'd prefer to add tests at some point so that any changes that break that functionality will be flagged by the testsuite.
Yeah, shell word splitting is just a pain to try to dance around.
We'd be happy to accept a refactor/chore PR cleaning those up, if you have the time! :) |
Completely agree with you :) I already got the itch, you don't need to push me too much to help around ^^ |
Fixed editor argument parsing by properly splitting whitespace with the same procedure used by shell.
Description
Fix for #1591
Motivation and Context
Shell word splitting is more complicated than splitting by whitespace.
The same fix may be applicable to other places in the application, if there are other places where a program with its arguments, given as a string, is split by whitespace and is executed in the same way.
Closes #1591
How Has This Been Tested?
See comments in #1591 for the process.
Checklist: