Content-Length: 301554 | pFad | https://github.com/starship/starship/pull/1595

04 fix: $EDITOR argument parsing by jRimbault · Pull Request #1595 · starship/starship · GitHub
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

fix: $EDITOR argument parsing #1595

Merged
merged 2 commits into from
Aug 21, 2020

Conversation

jRimbault
Copy link
Contributor

@jRimbault jRimbault commented Aug 17, 2020

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.

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Sorry, something went wrong.

@jRimbault jRimbault changed the title fix $EDITOR argument parsing fix: $EDITOR argument parsing Aug 17, 2020
@jRimbault jRimbault force-pushed the fix-spaces-in-editor branch from 4acd97c to 5293f8a Compare August 17, 2020 12:16
@jRimbault jRimbault force-pushed the fix-spaces-in-editor branch from 5293f8a to fd4c5db Compare August 17, 2020 13:51
@andytom andytom requested a review from a team August 18, 2020 17:34
Copy link
Contributor

@chipbuster chipbuster left a 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.

@jRimbault
Copy link
Contributor Author

jRimbault commented Aug 20, 2020

I didn't exactly "intuit" the use case of vim -m, I git blamed and looked up the commits and, as it happens, read the corresponding PR #772. To me that seems enough? If you wanted to be even more clever, I'd suggest adding tests and completely separating the logic from the side-effects.

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 (my executable has spaces in it.exe -w instead of "my executable has spaces in it.exe" -w), which can be documented as a gentle PEBCAK (imo, since git also shares this flaw).

Edit: as a sidenote in my quick cursory looking through the code I noticed a lot of duplicated code, and a few if cfg!(test) which could have been compile time conditionals instead of runtime checks. But outstanding contributing docs.

@chipbuster
Copy link
Contributor

I didn't exactly "intuit" the use case of vim -m, I git blamed and looked up the commits and, as it happens, read the corresponding PR #772. To me that seems enough?

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.

...which can be documented as a gentle PEBCAK (imo, since git also shares this flaw).

Yeah, shell word splitting is just a pain to try to dance around.

as a sidenote in my quick cursory looking through the code I noticed a lot of duplicated code, and a few if cfg!(test) which could have been compile time conditionals instead of runtime checks.

We'd be happy to accept a refactor/chore PR cleaning those up, if you have the time! :)

@jRimbault
Copy link
Contributor Author

Completely agree with you :)

I already got the itch, you don't need to push me too much to help around ^^

@chipbuster chipbuster merged commit 12c7877 into starship:master Aug 21, 2020
@jRimbault jRimbault deleted the fix-spaces-in-editor branch August 21, 2020 18:15
chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
Fixed editor argument parsing by properly splitting
whitespace with the same procedure used by shell.
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.

Can't open the $EDITOR when there are spaces in the path
2 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/starship/starship/pull/1595

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy