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

Added quoting for passwords in sample.app.ini and documentation #3395

Merged
merged 7 commits into from
Jan 27, 2018
Merged

Added quoting for passwords in sample.app.ini and documentation #3395

merged 7 commits into from
Jan 27, 2018

Conversation

marove2000
Copy link

This pull request is addressing issue #3380.

I've added quote hints in sample.app.ini and it's documentation, preventing wrong behavior when parsing the file if special characters are used in passwords.

Without quotes the password will be read wrong.

@lafriks lafriks added the type/docs This PR mainly updates/creates documentation label Jan 19, 2018
@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 19, 2018
@codecov-io
Copy link

codecov-io commented Jan 19, 2018

Codecov Report

Merging #3395 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3395      +/-   ##
==========================================
+ Coverage    35.6%   35.61%   +<.01%     
==========================================
  Files         281      281              
  Lines       40586    40586              
==========================================
+ Hits        14451    14453       +2     
+ Misses      23996    23994       -2     
  Partials     2139     2139
Impacted Files Coverage Δ
models/repo_list.go 67.18% <0%> (+1.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b627f11...0056704. Read the comment docs.

Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure backtick quoting is generic ? Or is it only working for mysql backend ?

@@ -118,7 +118,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- `HOST`: **127.0.0.1:3306**: Database host address and port.
- `NAME`: **gitea**: Database name.
- `USER`: **root**: Database username.
- `PASSWD`: **\<empty\>**: Database user password.
- `PASSWD`: **\`\<empty\>\`**: Database user password. Use \`your password\` for quoting, if you use special characters in the password.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look correct to quote <empty> ...

@@ -185,7 +185,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- `FROM`: **\<empty\>**: Mail from address, RFC 5322. This can be just an email address, or
the "Name" \<email@example.com\> format.
- `USER`: **\<empty\>**: Username of mailing user (usually the sender's e-mail address).
- `PASSWD`: **\<empty\>**: Password of mailing user.
- `PASSWD`: **\`\<empty\>\`**: Password of mailing user. Use \`your password\` for quoting, if you use special characters in the password.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quoting <empty> is unlikely correct, it's not a password, is an indication to the user that there's NOTHING written as a password...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this hint. Should be also enough to just write it in the comment. Backtick schould be the right quotation and works for both.

@lunny lunny added this to the 1.4.0 milestone Jan 19, 2018
@marove2000
Copy link
Author

I've removed the quotations from as suggested. Now it is only in the comment.

@thehowl
Copy link
Contributor

thehowl commented Jan 19, 2018

Classic german comma before the dependent clause ;) But I'm not gonna nitpick to that, because that's an incredibly minor issue and the sentence still works. LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 19, 2018
@lafriks lafriks merged commit bcd7f42 into go-gitea:master Jan 27, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants