From 469a9cfbdf28e8091ceadcfe75f56097493456b8 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 23 May 2024 20:22:45 +0100 Subject: [PATCH 1/2] GH-119169: Skip reversing sibling directories in `os.walk(topdown=False)` In `os.walk(topdown=False)`, don't bother reversing `walk_dirs`. This means that sibling directories are visited in a different order, but 1) that order is arbitrary and comes from `os.scandir()`, and 2) unlike in top-down mode, users can't influence which directories are visited or in what order. This change caused `test_walk_bottom_up` to fail. I think this test made assertions that were too specific and relied on `os.scandir()` returning things in a specific order, and the test code is pretty hard to understand once you get into the details. I've replaced it with a version of the same test from `test_pathlib_abc.py`. --- Lib/os.py | 3 +-- Lib/test/test_os.py | 50 +++++++++++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/Lib/os.py b/Lib/os.py index 7661ce68ca3be2..a07d07f8c26a29 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -429,8 +429,7 @@ def walk(top, topdown=True, onerror=None, followlinks=False): # Yield after sub-directory traversal if going bottom up stack.append((top, dirs, nondirs)) # Traverse into sub-directories - for new_path in reversed(walk_dirs): - stack.append(new_path) + stack.extend(walk_dirs) __all__.append("walk") diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 941fa2b2c5c87f..2b879863fd0704 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -1422,24 +1422,38 @@ def test_file_like_path(self): def test_walk_bottom_up(self): # Walk bottom-up. - all = list(self.walk(self.walk_path, topdown=False)) - - self.assertEqual(len(all), 4, all) - # We can't know which order SUB1 and SUB2 will appear in. - # Not flipped: SUB11, SUB1, SUB2, TESTFN - # flipped: SUB2, SUB11, SUB1, TESTFN - flipped = all[3][1][0] != "SUB1" - all[3][1].sort() - all[2 - 2 * flipped][-1].sort() - all[2 - 2 * flipped][1].sort() - self.assertEqual(all[3], - (self.walk_path, ["SUB1", "SUB2"], ["tmp1"])) - self.assertEqual(all[flipped], - (self.sub11_path, [], [])) - self.assertEqual(all[flipped + 1], - (self.sub1_path, ["SUB11"], ["tmp2"])) - self.assertEqual(all[2 - 2 * flipped], - self.sub2_tree) + sub2_path = os.path.join(self.walk_path, "SUB2") + seen_testfn = seen_sub1 = seen_sub11 = seen_sub2 = False + for path, dirnames, filenames in self.walk(self.walk_path, topdown=False): + if path == self.walk_path: + self.assertFalse(seen_testfn) + self.assertTrue(seen_sub1) + self.assertTrue(seen_sub2) + self.assertEqual(sorted(dirnames), ["SUB1", "SUB2"]) + self.assertEqual(filenames, ["tmp1"]) + seen_testfn = True + elif path == self.sub1_path: + self.assertFalse(seen_testfn) + self.assertFalse(seen_sub1) + self.assertTrue(seen_sub11) + self.assertEqual(dirnames, ["SUB11"]) + self.assertEqual(filenames, ["tmp2"]) + seen_sub1 = True + elif path == self.sub11_path: + self.assertFalse(seen_sub1) + self.assertFalse(seen_sub11) + self.assertEqual(dirnames, []) + self.assertEqual(filenames, []) + seen_sub11 = True + elif path == sub2_path: + self.assertFalse(seen_testfn) + self.assertFalse(seen_sub2) + self.assertEqual(sorted(dirnames), sorted(self.sub2_tree[1])) + self.assertEqual(sorted(filenames), sorted(self.sub2_tree[2])) + seen_sub2 = True + else: + raise AssertionError(f"Unexpected path: {path}") + self.assertTrue(seen_testfn) def test_walk_symlink(self): if not os_helper.can_symlink(): From cfc60ceb3bddd04bdd90485c11c25765f18395ce Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 04:39:58 +0100 Subject: [PATCH 2/2] Also undo reversals in `os.fwalk()` --- Lib/os.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/os.py b/Lib/os.py index 639ade30595d1d..ad7701e687d4d7 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -553,13 +553,15 @@ def _fwalk(stack, isbytes, topdown, onerror, follow_symlinks): toppath = path.join(toppath, toppath[:0]) # Add trailing slash. if entries is None: + if topdown: + dirs = dirs[::-1] stack.extend( (_fwalk_walk, (False, topfd, toppath + name, name, None)) - for name in dirs[::-1]) + for name in dirs) else: stack.extend( (_fwalk_walk, (False, topfd, toppath + name, name, entry)) - for name, entry in zip(dirs[::-1], entries[::-1])) + for name, entry in zip(dirs, entries)) __all__.append("fwalk")