On 2019-10-12 07:47, Daniel Shahaf wrote:
Yasuhito FUTATSUKI wrote on Sat, Oct 12, 2019 at 05:31:53 +0900:
Yes, it will fix local_missing_dir_endless_loop() itself correctly.
But the stack trace before fix indicate there is at least one problem
in svntest.verify.compare_and_display_lines().
Assume the file contents is broken here. This situation can be simulate
by patch like:
Index: subversion/tests/cmdline/tree_conflict_tests.py
===================================================================
--- subversion/tests/cmdline/tree_conflict_tests.py (revision 1868264)
+++ subversion/tests/cmdline/tree_conflict_tests.py (working copy)
@@ -1544,7 +1544,7 @@
contents = open(sbox.ospath('A1/B/lambda'), 'rb').readlines()
svntest.verify.compare_and_display_lines(
"A1/B/lambda has unexpectected contents", sbox.ospath("A1/B/lambda"),
- [ "This is the file 'lambda'.\n", "This is more content.\n"], contents)
+ [ b"This is the file 'lambda'.\n", b"This is not more content.\n"],
contents)
#######################################################################
then we will got fails.log, contains stack trace for unexpected exception
within the code to construct log message.
[[[
W: A1/B/lambda has unexpectected contents
W: EXPECTED svn-test-work/working_copies/tree_conflict_tests-26/A1/B/lambda
(match_all=True):
W: CWD: /home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline
Traceback (most recent call last):
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/svntest/main.py",
line 1931, in run
rc = self.pred.run(sandbox)
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/svntest/testcase.py",
line 178, in run
result = self.func(sandbox)
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/tree_conflict_tests.py",
line 1547, in local_missing_dir_endless_loop
[ b"This is the file 'lambda'.\n", b"This is not more content.\n"],
contents)
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/svntest/verify.py",
line 503, in compare_and_display_lines
expected.display_differences(message, label, actual)
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/svntest/verify.py",
line 154, in display_differences
display_lines(message, self.expected, actual, e_label, label)
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/tests/cmdline/svntest/verify.py",
line 474, in display_lines
logger.warn('| ' + x.rstrip())
TypeError: can only concatenate str (not "bytes") to str
FAIL: tree_conflict_tests.py 26: endless loop when resolving local-missing dir
]]]
This is caused by mixing bytes object drived from file contents and str
object to construct log message.
I agree: this FAIL indicates str and bytes are mixed. My question is:
Why do you think svntest.verify.compare_and_display_lines() needs to be
changed? That function's names implies it deals with text files, so,
why should compare_and_display_lines() support the case that its third
and fourth parameters (EXPECTED and ACTUAL) are both lists of bytes objects?
In other words, why would changing «'rb'» to «'r'» on line 1544 —
without changing the str literals to bytes literals on line 1547 — not
be a correct solution?
Ah, I thought strict comparison is needed here because of raw mode file I/O.
Index: subversion/tests/cmdline/tree_conflict_tests.py
===================================================================
--- subversion/tests/cmdline/tree_conflict_tests.py (revision 1868264)
+++ subversion/tests/cmdline/tree_conflict_tests.py (working copy)
@@ -1518,7 +1518,7 @@
sbox.simple_move('A/B', 'A/B2')
sbox.simple_commit()
sbox.simple_update()
- main.file_append_binary(sbox.ospath("A/B2/lambda"), "This is more
content.\n")
+ main.file_append_binary(sbox.ospath("A/B2/lambda"), "This is more
content.\r\n")
sbox.simple_commit()
sbox.simple_update()
@@ -1544,7 +1544,7 @@
contents = open(sbox.ospath('A1/B/lambda'), 'rb').readlines()
svntest.verify.compare_and_display_lines(
"A1/B/lambda has unexpectected contents", sbox.ospath("A1/B/lambda"),
- [ "This is the file 'lambda'.\n", "This is more content.\n"], contents)
+ [ b"This is the file 'lambda'.\n", b"This is more content.\n"], contents)
#######################################################################
Above patch will make local_missing_dir_endless_loop test fail because of
strictness of comparison. However,
Index: subversion/tests/cmdline/tree_conflict_tests.py
===================================================================
--- subversion/tests/cmdline/tree_conflict_tests.py (revision 1868264)
+++ subversion/tests/cmdline/tree_conflict_tests.py (working copy)
@@ -1518,7 +1518,7 @@
sbox.simple_move('A/B', 'A/B2')
sbox.simple_commit()
sbox.simple_update()
- main.file_append_binary(sbox.ospath("A/B2/lambda"), "This is more
content.\n")
+ main.file_append_binary(sbox.ospath("A/B2/lambda"), "This is more
content.\r\n")
sbox.simple_commit()
sbox.simple_update()
@@ -1541,7 +1541,7 @@
# If everything works as expected the resolver will recommended a
# resolution option and 'svn' will resolve the conflict automatically.
# Verify that 'A1/B/lambda' contains the merged content:
- contents = open(sbox.ospath('A1/B/lambda'), 'rb').readlines()
+ contents = open(sbox.ospath('A1/B/lambda'), 'r').readlines()
svntest.verify.compare_and_display_lines(
"A1/B/lambda has unexpectected contents", sbox.ospath("A1/B/lambda"),
[ "This is the file 'lambda'.\n", "This is more content.\n"], contents)
this will make it succcess because of "universal newline" feature on Python.
If textual comparison is sufficient here, it is right to open file
text mode (with suitable, unified set of `encoding', `errors', and `newline'
parameter). Otherwise, if strict comparison is needed, we must avoid unwanted,
not one-on-one corresponding conversion from bytes to str applied by Python.
In the latter case, it may be rather incorrect to use
compare_and_display_lines().
Hope I'm clearer this time. If not, I'd be happy to clarify.
Cheers,
Daniel
Cheers,
--
Yasuhito FUTATSUKI <futat...@poem.co.jp>
