gh-139352: prevent altering PYTHON_HISTORY file when running REPL tests
#139380
+171
−52
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
picnixz
requested review from
pablogsal,
lysnikolaou and
ambv
as code owners
|
NVM, there are other places that alter this file. |
|
Yeah, I'll do that tomorrow to catch the possible issues. I can also make it part of the "ENV CHANGED" check |
|
Urgh, |
|
If possible, I'd like to suggest a cleaner way to implement EDIT: for now, all regular tests pass and don't modify the history. |
|
This work but will print a lot of messages is that expected? |
|
update...seems not work on mac firsttime |
|
Well, I don't have a Mac for testing zipfile. I don't have an issue on Linux with zipfile though (I actually had NO issue about wiping the history file, only about the history being altered so I don't really understand. Which test is actually removing the history file? all "errors" are only about the file being altered, not being deleted) |
not sure...will help to check it on mac later. on branch fix/repl/history-alteration-139352 ➜ cpython git:(fix/repl/history-alteration-139352) cat ~/.python_history after the cat ~/.python_history # nothing |
|
Ok wait a moment. |
|
Can you recheck what happens for you if run |
doing git pull make test (not all if you need will paste all) |
|
FTR, I've tested both |
|
after make test ➜ cpython git:(fix/repl/history-alteration-139352) cat ~/.python_history ./python.exe -m test -j12 is on going |
I need to know which test actually removed the history. Is the history file empty after the tests, or is it just gone (that is |
|
j12 is the same... |
yes help checking now |
|
@picnixz this guy ➜ cpython git:(fix/repl/history-alteration-139352) ./python.exe
➜ cpython git:(fix/repl/history-alteration-139352) cat ~/.python_history
|
|
Huh, so actually what might happen is that we're trying to write to the history file but the latter is somehow overwritten in a bad manner. I may think of one reason, that is: def read_history_file(self, filename: str = gethistoryfile()) -> None: ...
def write_history_file(self, filename: str = gethistoryfile()) -> None: ...
def append_history_file(self, filename: str = gethistoryfile()) -> None: ...Those functions in diff --git a/Lib/_pyrepl/readline.py b/Lib/_pyrepl/readline.py
index 23b8fa6b9c7..471a955e3a4 100644
--- a/Lib/_pyrepl/readline.py
+++ b/Lib/_pyrepl/readline.py
@@ -431,6 +431,7 @@ def read_history_file(self, filename: str = gethistoryfile()) -> None:
# history item: we use \r\n instead of just \n. If the history
# file is passed to GNU readline, the extra \r are just ignored.
history = self.get_reader().history
+ print("history = ", filename, "\n", history)
with open(os.path.expanduser(filename), 'rb') as f:
is_editline = f.readline().startswith(b"_HiStOrY_V2_")
@@ -457,6 +458,7 @@ def read_history_file(self, filename: str = gethistoryfile()) -> None:
def write_history_file(self, filename: str = gethistoryfile()) -> None:
maxlength = self.saved_history_length
history = self.get_reader().get_trimmed_history(maxlength)
+ print("history = ", filename, "\n", history)
f = open(os.path.expanduser(filename), "w",
encoding="utf-8", newline="\n")
with f:
@@ -469,6 +471,7 @@ def append_history_file(self, filename: str = gethistoryfile()) -> None:
saved_length = self.get_history_length()
length = self.get_current_history_length() - saved_length
history = reader.get_trimmed_history(length)
+ print("history = ", filename, "\n", history)
f = open(os.path.expanduser(filename), "a",
encoding="utf-8", newline="\n")
with f:To apply the patch, first write in your console: cat <<-EOM > repl.patchthen, copy-paste the following: You should have a file named $ git apply repl.patchNow, run the following commands: $ echo "1\n2\n3\n" > ~/.python_history
$ cat ~/.python_history
$ ./python -m test test_cmd_line_script -m test_repl_stderr_flush -v
$ cat ~/.python_historyand paste the entire output here. |
|
|
is there somehing else I can help here? |
|
I think there is something with macOS and editline that is different than on Linux. I don't have this What might happen however is that cc @ambv @pablogsal |
git grep "HiStOrY_V2" Lib/_pyrepl/readline.py: is_editline = f.readline().startswith(b"HiStOrY_V2") |
|
linux for this patch works well |
|
a bit dig users especialy for mac users the deafault readline maybe when use editline the clean history happens the quite fix diff(maybe others) diff --git a/Lib/_pyrepl/readline.py b/Lib/_pyrepl/readline.py
index 23b8fa6b9c..1ac1f88b02 100644
--- a/Lib/_pyrepl/readline.py
+++ b/Lib/_pyrepl/readline.py
@@ -457,9 +457,21 @@ def read_history_file(self, filename: str = gethistoryfile()) -> None:
def write_history_file(self, filename: str = gethistoryfile()) -> None:
maxlength = self.saved_history_length
history = self.get_reader().get_trimmed_history(maxlength)
- f = open(os.path.expanduser(filename), "w",
- encoding="utf-8", newline="\n")
+ filename_expanded = os.path.expanduser(filename)
+ # Simple check: if existing file starts with _HiStOrY_V2_, preserve editline format
+ use_editline_format = False
+ try:
+ with open(filename_expanded, 'rb') as f:
+ if f.readline().startswith(b"_HiStOrY_V2_"):
+ use_editline_format = True
+ except (FileNotFoundError, PermissionError, OSError):
+ pass
+
+ f = open(filename_expanded, "w", encoding="utf-8", newline="\n")
with f:
+ if use_editline_format:
+ # Preserve editline format
+ f.write("_HiStOrY_V2_\n")
for entry in history:
entry = entry.replace("\n", "\r\n") # multiline history support
f.write(entry + "\n")
|
this dirty can fix ➜ cpython git:(fix/repl/history-alteration-139352) ✗ cat ~/.python_history |
|
@yihong0618 Thanks for the investigation. I included a modified version of the patch. Please try this branch again. |
error: Traceback (most recent call last): |
|
Fixed, sorry! |
not fully right seems have some format problem history broken |
|
and I checked my patch is not fully right too This keep the history only we have if we do not have it like ./python.exe -m unittest test.test_cmd_line will clean the history too diff --git a/Lib/_pyrepl/readline.py b/Lib/_pyrepl/readline.py
index 23b8fa6b9c..1ac1f88b02 100644
--- a/Lib/_pyrepl/readline.py
+++ b/Lib/_pyrepl/readline.py
@@ -457,9 +457,21 @@ def read_history_file(self, filename: str = gethistoryfile()) -> None:
def write_history_file(self, filename: str = gethistoryfile()) -> None:
maxlength = self.saved_history_length
history = self.get_reader().get_trimmed_history(maxlength)
- f = open(os.path.expanduser(filename), "w",
- encoding="utf-8", newline="\n")
+ filename_expanded = os.path.expanduser(filename)
+ # Simple check: if existing file starts with _HiStOrY_V2_, preserve editline format
+ use_editline_format = False
+ try:
+ with open(filename_expanded, 'rb') as f:
+ if f.readline().startswith(b"_HiStOrY_V2_"):
+ use_editline_format = True
+ except (FileNotFoundError, PermissionError, OSError):
+ pass
+
+ f = open(filename_expanded, "w", encoding="utf-8", newline="\n")
with f:
+ if use_editline_format:
+ # Preserve editline format
+ f.write("_HiStOrY_V2_\n")
for entry in history:
entry = entry.replace("\n", "\r\n") # multiline history support
f.write(entry + "\n") |
picnixz
force-pushed
the
fix/repl/history-alteration-139352
branch
from
400e295 to
1c7e2d9
Compare
|
It should be fixed for the format. For the rest idk |
@picnixz works well but same as my patch This keep the history only we have HiStOrY_V2 if we do not have it like |
|
@picnixz dig more.... this patch fix the left issue diff --git a/Modules/readline.c b/Modules/readline.c
index 630a687999..915f12121e 100644
--- a/Modules/readline.c
+++ b/Modules/readline.c
@@ -317,6 +317,9 @@ readline_read_history_file_impl(PyObject *module, PyObject *filename_obj)
static int _history_length = -1; /* do not truncate history by default */
+/* Forward declaration */
+static int _py_get_history_length_lock_held(void);
+
/* Exported function to save a readline history file */
/*[clinic input]
@@ -354,6 +357,13 @@ readline_write_history_file_impl(PyObject *module, PyObject *filename_obj)
return NULL;
}
}
+ // Check if we have any history to write
+ int current_history_length = _py_get_history_length_lock_held();
+ // If current history is empty, don't overwrite existing file
+ if (current_history_length == 0) {
+ Py_XDECREF(filename_bytes);
+ Py_RETURN_NONE;
+ }
errno = err = write_history(filename);
int history_length = FT_ATOMIC_LOAD_INT_RELAXED(_history_length);
if (!err && history_length >= 0)
with your fix and the diff patch it fix all and maybe since its two issues here one about |
|
I never dabbled with the C implementation of readline so I would prefer having someone who is more familiar with it to confirm whether this is the appropriate call. @ambv and @pablogsal: could you perhaps help us here? |
ambv
requested changes
Oct 6, 2025
|
|
||
| @staticmethod | ||
| def _is_editline_history(filename: str | IO[bytes]) -> bool: | ||
| if isinstance(filename, str): |
There was a problem hiding this comment.
Don't implement the str bit. Now there's two functions in one.
Comment on lines
+474
to
+475
| if is_editline: | ||
| f.write(f"{_EDITLINE_MARKER}\n") |
There was a problem hiding this comment.
This is incorrect and this change is irrelevant to the bug you're fixing. The editline format is somewhat different, a file written by us will not be readable by editline anyway, and now you'll make pyrepl mangle unicode in its own history until you delete the history file.
| self.next_single_filename: StrPath | None = None | ||
|
|
||
| history_file = os.path.join(os.path.expanduser('~'), '.python_history') | ||
| self.__history_file = history_file |
There was a problem hiding this comment.
Nit: don't use double underscores. We don't use them anywhere in libregrtest. A single underscore's better.
|
When you're done making the requested changes, leave the comment: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue with the tests is that those that don't specify an environment actually use
-Iand the current environment. Even if the latter is modified with a differentPYTHON_HISTORY, because of-I, it will be ignored bygethistoryfile().I decided that it was cleaner to assume that
Noneenvironment means "use a copy of the current one" and that the it shouldn't be possible to specify-Iat the same time if we internally addPYTHON_HISTORYourselves.I believe that we need a better mechanism, but for now, I want to prevent the test suite to overwrite
~/.python_history(this could be especially annoying for those who actually want to install python system-wide; alternatively, we could make sure to restore the history file at the end of the test).make testalters~/.python_history#139352