You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "enjoy-binbin (via GitHub)" <gi...@apache.org> on 2023/04/20 08:13:09 UTC
[GitHub] [incubator-kvrocks] enjoy-binbin opened a new pull request, #1397: Fix CONFIG REWRITE append duplicate newlines
enjoy-binbin opened a new pull request, #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397
In the old code, everytime we perform a config rewrite, an
additional newline is appended, see #1396
Because iostream::eof will only return true after reading the
end of the stream. It does not indicate, that the next read will
be the end of the stream.
So everytime config rewrite is performed, the old loop will
append a newline in `lines`, and then we will append it to the
conf file.
In addition, the same modification has been made to other similar
places. This PR fixes #1396
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] enjoy-binbin commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1515913035
I tested it locally (with the CONFIG REWRITE)
and the ref: https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-i-e-while-stream-eof-cons
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#discussion_r1172266740
##########
src/config/config.cc:
##########
@@ -829,8 +829,8 @@ Status Config::Rewrite() {
std::ifstream file(path_);
if (file.is_open()) {
std::string raw_line;
- while (!file.eof()) {
- std::getline(file, raw_line);
+ while (std::getline(file, raw_line)) {
+ if (file.eof()) break;
Review Comment:
If the text file has no newline after the last line, this line will be ignored.
e.g.
```python
file.write("a\nb\nc"); # the `c` line will be ignored
```
Hence I think it is not a good change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1515955590
Here is a simple test to demostrate this point:
```
❯ cat y.cpp
#include <fstream>
#include <iostream>
#include <string>
int main() {
std::ifstream file("hello.txt");
std::string line;
while (std::getline(file, line)) {
if (file.eof()) break;
std::cout << "> " << line << std::endl;
}
}
❯ python -c "file=open('hello.txt', 'w');file.write('a\nb\nc');file.close()"
❯ cat hello.txt
a
b
c%
❯ ./y
> a
> b
❯
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1516034299
> @PragmaTwice thanks for the detailed explanation! do you have any suggestions? i may need some time to figure it out do you think if i put the `if (file.eof()) break;` in the last will help?
Not sure the eof check is necessary, maybe something like `while(file.good() && getline(...))` is ok.
The call to `getline` will return the stream itself, so `operator bool` of the stream is called, and it is equal to `!stream.fail()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1516282176
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1515915191
@enjoy-binbin Thanks for your efforts. ❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] enjoy-binbin commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1516011357
@PragmaTwice thanks for the detailed explanation! do you have any suggestions? i may need some time to figure it out
do you think if i put the `if (file.eof()) break;` in the last will help?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#discussion_r1172266740
##########
src/config/config.cc:
##########
@@ -829,8 +829,8 @@ Status Config::Rewrite() {
std::ifstream file(path_);
if (file.is_open()) {
std::string raw_line;
- while (!file.eof()) {
- std::getline(file, raw_line);
+ while (std::getline(file, raw_line)) {
+ if (file.eof()) break;
Review Comment:
If the text file has no newline after the last line, this line will be ignored.
e.g.
```
file.write("a\nb\nc"); # the `c` line will be ignored
```
Hence I think it is not a good change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] enjoy-binbin commented on pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#issuecomment-1516064593
i change to use `while(file.good() && getline(...))` for now, i also tested it in local, it is fine
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1397: Fix CONFIG REWRITE append duplicate newlines
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1397:
URL: https://github.com/apache/incubator-kvrocks/pull/1397#discussion_r1172429808
##########
src/config/config.cc:
##########
@@ -726,8 +726,7 @@ Status Config::Load(const CLIOptions &opts) {
std::string line;
int line_num = 1;
- while (!in->eof()) {
- std::getline(*in, line);
+ while (in.good() && std::getline(*in, line)) {
Review Comment:
```suggestion
while (in->good() && std::getline(*in, line)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org