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: read_history() multiple times will add repeat histories to histo… #101

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Conversation

flyxyz123
Copy link
Contributor

@flyxyz123 flyxyz123 commented Aug 14, 2024

…ry lists

Issue Description:

When sdcv found multiple items, whatever your choice is, sdcv will add double the current history entries to history file. For example, if current history is "a\nb", you search akjk and there's multiple results, whatever you choose, even -1, after this is done, the history file will be rewritten to "a\nb\nakjk\na\nb", note here \n is newline character. So if you have 500 lines of history, you search akjk and there's multiple results, you choose -1, after done there's 1001 lines of history.

How to reproduce:

You can download this dictionary file
https://github.com/skywind3000/ECDICT/releases/download/1.0.28/ecdict-stardict-28.zip and put into your dictionary directory, on Arch, with AUR, you can install from https://aur.archlinux.org/packages/stardict-ecdict. Then, make sure you also add the dictionary name to ~/.config/sdcv_ordering if you have one. Add some lines to your history file if you do not have. Then search for "akjk" with sdcv, e,g, sdcv akjk, then it will prompt you to choose, you can choose -1, then ctrl-d to exit. Expected result is history file add akjk at the end. Actual result is history file now contain original content + akjk + original content duplicate as I described in the issue description.

Fix and reasons:

I'm a hobbyist and I'm not a professional, I haven't use C++ for years so many of my writings is very likely wrong. After some trail and error, I found that call read_history() multiple times will add repeat histories to history lists. In the commit d2327e2, a new IReadLine object is created (note name changes, also note I know this description of a IReadLine object is wrong). Here's a permalink:

std::unique_ptr<IReadLine> choice_readline(create_readline_object());
. The problem of this new IReadLine object choice_readline is it called read_history() again from ./src/readline.cpp constructor, because there's already a IReadLine object io constructed at ./src/sdcv.cpp. When read_history() is called twice, there's a "history lists" read from history file first then append from history file again, so when you destruct IReadLine object with write_history() in ./src/readline.cpp, the history file contain duplicate content after write. Here are permalinks:
read_history(histname.c_str());
, and
write_history(histname.c_str());
.

So my fix is to just to use io IReadLine object and not to create a new choice_readline object.

Misc:

During my trial and errors process, I made an example code to show read_history()'s weird behavior. I did not dig deeper, I just guess maybe there's some kind of werid history list as mentioned in https://tiswww.cwru.edu/php/chet/readline/history.html#History-List-Management.

Here's the example code a.c, note you need to include stdio.h, readline/history.h, and readline/readline.h. I did not include them here because commit message seems will make them comment.

...
int main (void)
{
	rl_readline_name="learn_readline";
	using_history();
	read_history("/home/xyz/test/learn_readline/history.txt");
	write_history("/home/xyz/test/learn_readline/history2.txt");
	{
		rl_readline_name="learn_readline";
		using_history();
		read_history("/home/xyz/test/learn_readline/history.txt");
		write_history("/home/xyz/test/learn_readline/history3.txt");
	}
	write_history("/home/xyz/test/learn_readline/history4.txt");
	return 0;
}

Here's the content of history.txt:

a
b

After build and run, as you can guess, history2.txt is same as history.txt. But history3.txt and history4.txt content are:

a
b
a
b

@flyxyz123
Copy link
Contributor Author

flyxyz123 commented Aug 14, 2024

I want to emphasize that I'm not a professional. I'm not familiar with C++ and GNU readline. So maybe I'm totally wrong. For example, maybe it is a bug in GNU readline? I don't know. Here's just a proposal for a potential fix or maybe workaround. There are maybe better ways. I also forget to mention that I have Arch Linux's latest GNU readline 8.2.013 instealled, sdcv is also installed with the latest version 0.5.5.

…ry lists

Issue Description:

When sdcv found multiple items, whatever your choice is, sdcv will add
double the current history entries to history file. For example, if
current history is "a\nb", you search akjk and there's multiple results,
whatever you choose, even -1,  after this is done, the history file will
be rewritten to "a\nb\nakjk\na\nb", note here \n is newline character.
So if you have 500 lines of history, you search akjk and there's
multiple results, you choose -1, after done there's 1001 lines of
history.

How to reproduce:

You can download this dictionary file
https://github.com/skywind3000/ECDICT/releases/download/1.0.28/ecdict-stardict-28.zip
and put into your dictionary directory, on Arch, with AUR, you can
install from https://aur.archlinux.org/packages/stardict-ecdict. Then,
make sure you also add the dictionary name to ~/.config/sdcv_ordering if
you have one. Add some lines to your history file if you do not have.
Then search for "akjk" with sdcv, e,g, `sdcv akjk`, then it will prompt
you to choose, you can choose -1, then ctrl-d to exit. Expected result
is history file add akjk at the end. Actual result is history file now
contain original content + akjk + original content duplicate as I
described in the issue description.

Fix and reasons:

I'm a hobbyist and I'm not a professional, I haven't use C++ for years
so many of my writings is very likely wrong. After some trail and error,
I found that call read_history() multiple times will add repeat
histories to history lists. In the commit d2327e2, a new IReadLine
object is created (note name changes, also note I know this description
of a IReadLine object is wrong). Here's a permalink:
https://github.com/Dushistov/sdcv/blob/49c8094b53b7dd90efeabeaf276752a650a0f931/src/libwrapper.cpp#L418.
The problem of this new IReadLine object `choice_readline` is it called
read_history() again from ./src/readline.cpp constructor, because
there's already a IReadLine object `io` constructed at ./src/sdcv.cpp.
When read_history() is called twice, there's a "history lists" read from
history file first then append from history file again, so when you
destruct IReadLine object with write_history() in ./src/readline.cpp,
the history file contain duplicate content after write. Here are
permalinks:
https://github.com/Dushistov/sdcv/blob/49c8094b53b7dd90efeabeaf276752a650a0f931/src/readline.cpp#L88,
and
https://github.com/Dushistov/sdcv/blob/49c8094b53b7dd90efeabeaf276752a650a0f931/src/readline.cpp#L94.

So my fix is to just to use `io` IReadLine object and not to create a
new `choice_readline` object.

Misc:

During my trial and errors process, I made an example code to show
read_history()'s weird behavior. I did not dig deeper, I just guess
maybe there's some kind of werid history list as mentioned in
https://tiswww.cwru.edu/php/chet/readline/history.html#History-List-Management.

Here's the example code a.c, note you need to include stdio.h,
readline/history.h, and readline/readline.h. I did not include them here
because commit message seems will make them comment.
```c
...
int main (void)
{
	rl_readline_name="learn_readline";
	using_history();
	read_history("/home/xyz/test/learn_readline/history.txt");
	write_history("/home/xyz/test/learn_readline/history2.txt");
	{
		rl_readline_name="learn_readline";
		using_history();
		read_history("/home/xyz/test/learn_readline/history.txt");
		write_history("/home/xyz/test/learn_readline/history3.txt");
	}
	write_history("/home/xyz/test/learn_readline/history4.txt");
	return 0;
}
```
Here's the content of history.txt:
```
a
b
```
After build and run, as you can guess, history2.txt is same as
history.txt. But history3.txt and history4.txt content are:
```
a
b
a
b
```

Signed-off-by: Xiao Pan <[email protected]>
@Dushistov Dushistov merged commit beebb0f into Dushistov:master Aug 15, 2024
2 checks passed
@Dushistov
Copy link
Owner

Thank you for fix.

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.

2 participants