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 CONFIG REWRITE append duplicate newlines #1397

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

enjoy-binbin
Copy link
Member

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

In the old code, everytime we perform a config rewrite, an
additional newline is appended, see apache#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 apache#1396
@enjoy-binbin
Copy link
Member Author

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

@git-hulk
Copy link
Member

@enjoy-binbin Thanks for your efforts. ❤️

git-hulk
git-hulk previously approved these changes Apr 20, 2023
torwig
torwig previously approved these changes Apr 20, 2023
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

src/config/config.cc Outdated Show resolved Hide resolved
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Thanks for your patch, but seems changes are required.

@PragmaTwice
Copy link
Member

PragmaTwice commented Apr 20, 2023

Here is a simple test to demonstrate 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

❯

@enjoy-binbin
Copy link
Member Author

@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?

@PragmaTwice
Copy link
Member

@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().

@enjoy-binbin enjoy-binbin dismissed stale reviews from torwig and git-hulk via bf58fa9 April 20, 2023 10:04
@enjoy-binbin
Copy link
Member Author

i change to use while(file.good() && getline(...)) for now, i also tested it in local, it is fine

src/config/config.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit 1f8fa1a into apache:unstable Apr 20, 2023
@enjoy-binbin enjoy-binbin deleted the fix_eof_getline branch April 20, 2023 13:00
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.

config rewrite bug
4 participants