Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates scripts/update_lit_checks.py to properly handle and escape paths containing spaces when running commands, and adds a corresponding unit test suite in test/unit/test_update_lit_checks_paths.py. The review feedback points out a potential bug in the path replacement logic where sequential .replace() calls could lead to recursive replacements if the paths themselves contain placeholder substrings, and suggests a single-pass regular expression replacement as a safer alternative.
| def replace_paths(text): | ||
| return text.replace('%s', test).replace('%S', os.path.dirname(test)).replace('%t', tmp) |
There was a problem hiding this comment.
Using sequential .replace() calls can lead to unexpected behavior if one of the replacement values (such as test or tmp paths) contains another placeholder substring (e.g., if the test path contains %t or %S). In such cases, the subsequent .replace() calls will perform recursive replacements on the already-replaced path, corrupting the final command.
Using a single-pass regular expression replacement ensures that each placeholder is replaced exactly once and prevents recursive replacement bugs.
| def replace_paths(text): | |
| return text.replace('%s', test).replace('%S', os.path.dirname(test)).replace('%t', tmp) | |
| def replace_paths(text): | |
| replacements = { | |
| '%s': test, | |
| '%S': os.path.dirname(test), | |
| '%t': tmp | |
| } | |
| return re.sub(r'%[sSt]', lambda m: replacements[m.group()], text) |
No description provided.