-
Notifications
You must be signed in to change notification settings - Fork 815
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 .netrc parsing #7698
Fix .netrc parsing #7698
Conversation
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.
thanks for the PR
there would still be failing tests
see
FAIL! : TestNetrcParser::testValidNetrc() Compared values are not the same
Actual (parser.find("frob")) : std::pair(""user", "'space")
Expected (qMakePair(QString("user with spaces"), QString("space pwd"))): std::pair("user with spaces", "space pwd")
Loc: [/home/mgallien/work/nextcloud/desktop/test/testnetrcparser.cpp(60)]
I also needed to remove the QEXPECT_FAIL
instances from test/testnetrcparser.cpp
file
can you add those changes to the PR ?
I think this is as good as I can do. The only test that still fail is username and passwords containing whitespaces, and I think that would require a bigger refactoring. I've commented out that test for now to enable the rest of them. |
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.
thanks @fredrik-eriksson
can you fix the missing signed off comment and fix my inline comment ?
apart from that, I fully agree that your change can be merged as is and is already a big improvement
ac7a2a3
to
1aa6b2d
Compare
Thanks for the feedback, I hope I got it right this time 🙂 |
Fixes: nextcloud#7177 Signed-off-by: Fredrik Eriksson <[email protected]>
Also fixes handling of empty netrc. Signed-off-by: Fredrik Eriksson <[email protected]>
Test for spaces in usernames and passwords is disabled for now as it's not supported by current implementation. Signed-off-by: Fredrik Eriksson <[email protected]>
@fredrik-eriksson thanks for the very fast review cycles |
/backport to stable-3.15 |
Fixes: #7177
It seems that the current Tokenizer-implementation splits the netrc tokens on a (string) separator of " \n\t" instead of splitting on any of the whitespace characters which leads to the issue described in #7177.
This implementation splits the content of netrc on any whitespace character using a regex instead, but should otherwise use the same strategy for parsing. I'm not very familiar with C++ (or QT) and does not completely understand the code, so although this solution seems to work for me, I would appreciate a review 🙂
Thanks!