-
Notifications
You must be signed in to change notification settings - Fork 698
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 parsing of password-command option (#6268) #9002
Fix parsing of password-command option (#6268) #9002
Conversation
Good job here, thank you! Would it be possible to add a test? |
1927d8c
to
b78c95a
Compare
@ulysses4ever I added (well, extended) a test, and updated the users guide. |
bump please review when able :) |
@Kleidukos also affects |
@frasertweedale please, add a changelog as described here. |
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.
Predicated on the changelog, this looks good to me, thanks!
Also, I think it'd be nice to have manual QA for this change. I'll add a note in the OP. |
The password-command option does not parse its value correctly. Quotes are ignored, making many kinds of commands impossible to express (e.g. `sh -c "foo | bar"`). Also, `cabal user-config` treats the argument list as a *list of option values*, rather than a *value that is a list*. As a consequence, `cabal user-config update` corrupts the value in the config file. Fix these issues by parsing the command as a space separated list of tokens, and changing the getter to `unwords` the value and return a *singleton* list. Also update the argument placeholder from `PASSWORD` to `COMMAND`. Fixes: haskell#6268
b78c95a
to
95f48ad
Compare
@ulysses4ever I added a |
Thank you! Waiting for a second review… |
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.
Looks reasonable to me
Something went wrong with mergify. Can someone advise on next steps? I logged into mergify (never used it previously) but it couldn't access the |
@frasertweedale no problems, thanks |
Here are the results of QA:
Running => password-command was changed, bug exists
Running Same as before, so QA is passed 🥇 Edit: hold on, I checked the wrong file, sorry. The following change occurs in my config file after running Is this the correct behavior? @frasertweedale |
@jgotoh thanks a bunch! |
This is expected. The values are semantically equivalent. It's an artifact of the token parsing compared to the pretty-printing of the option values. There are two ways of representing a simple token: with quotes, or without. So unless we represent which variant was used in the option values, there is no way to ensure the string representation round-trips. |
Awesome, thank you very much for your quick answer and your detailed description. Then QA is passed here! :) |
Bugfix -- qualifies for a backport, I think. |
@mergify backport 3.10 |
✅ Backports have been created
|
The password-command option does not parse its value correctly.
Quotes are ignored, making many kinds of commands impossible to
express (e.g.
sh -c "foo | bar"
). Also,cabal user-config
treats the argument list as a list of option values, rather than a
value that is a list. As a consequence,
cabal user-config update
corrupts the value in the config file.Fix these issues by parsing the command as a space separated list of
tokens, and changing the getter to
unwords
the value and return asingleton list. Also update the argument placeholder from
PASSWORD
toCOMMAND
.Fixes: #6268
Manual QA Notes
Please include the following checklist in your PR:
Bonus points for added automated tests!