Repository navigation
fix(link-checker): preserve changed filenames with special characters - #1429
BichengWang wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the link checker tool when identifying changed files in a Git repository. By switching to NUL-delimited output from Git, the tool now correctly handles filenames that were previously skipped or incorrectly processed due to special characters or unconventional formatting. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
the
status:awaiting review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the git diff command in tools/check_all_links.py to use the -z flag and split paths on null bytes, ensuring robust handling of special characters and newlines in filenames. Corresponding test cases were updated, and new tests were added to verify special character and Unicode filename handling. Feedback on the changes suggests explicitly specifying encoding="utf-8" in subprocess.check_output instead of using text=True to prevent potential UnicodeDecodeError or filename corruption on systems where the default encoding is not UTF-8.
|
Thanks @BichengWang , nice catch. Reproduced locally: on main, --changed silently skips a file like quickstarts/指南.md because git prints it quoted ("quickstarts/\346\214\207\345\215\227.md") and the .md check fails. With your branch it's picked up. Also checked the UTF-8 decoding follow-up: the new locale test fails against the previous commit (None != ['quickstarts/指南.md']) and passes on 7bf7fa8, so it's a real regression test. 21/21 pass. LGTM. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the internal link-checking tool (tools/check_all_links.py) to use null-terminated output (-z) when executing git diff, which ensures that filenames containing special characters, newlines, or Unicode are correctly preserved and decoded. It also updates existing tests and adds new test cases to verify this behavior. However, according to the repository style guide, changes to internal tooling are only accepted if they address a concrete bug actively affecting files on the main branch rather than theoretical edge cases. Furthermore, the style guide explicitly prohibits adding or maintaining unit tests for internal helper scripts under tools/, meaning the newly added test cases should be removed.
| def test_changed_files_preserves_special_characters(self) -> None: | ||
| """Git paths must not be quoted, split on newlines, or stripped.""" | ||
| paths = [ | ||
| "quickstarts/指南.ipynb", | ||
| "quickstarts/line\nbreak.md", | ||
| "quickstarts/tab\tname.md", | ||
| 'quickstarts/quoted"name.md', | ||
| " leading-space.md", | ||
| ] | ||
| with mock.patch( | ||
| "tools.check_all_links.subprocess.check_output", | ||
| return_value="\0".join(paths + ["tools/check_all_links.py", ""]), | ||
| ) as git_diff: | ||
| self.assertEqual(get_changed_files_from_git(self.repo_root), paths) | ||
| self.assertEqual( | ||
| git_diff.call_args.args[0], | ||
| ["git", "diff", "--name-only", "-z", "upstream/main...HEAD"], | ||
| ) | ||
|
|
||
| def test_changed_cli_audits_unicode_filename(self) -> None: | ||
| """A changed Unicode filename must not hide a broken link from the CLI.""" | ||
| target = self.quickstarts_dir / "指南.md" | ||
| target.write_text("[Broken](missing.md)\n", encoding="utf-8") | ||
| output = io.StringIO() | ||
| with ( | ||
| mock.patch( | ||
| "tools.check_all_links.subprocess.check_output", | ||
| return_value="quickstarts/指南.md\0", | ||
| ), | ||
| mock.patch.object( | ||
| sys, | ||
| "argv", | ||
| ["check_all_links.py", "--repo-dir", str(self.repo_root), "--changed"], | ||
| ), | ||
| contextlib.redirect_stdout(output), | ||
| self.assertRaises(SystemExit) as exit_context, | ||
| ): | ||
| main() | ||
| self.assertEqual(exit_context.exception.code, 1, output.getvalue()) | ||
| self.assertIn("quickstarts/指南.md", output.getvalue()) | ||
| self.assertIn("File does not exist: missing.md", output.getvalue()) | ||
|
|
||
| def test_changed_files_decodes_utf8_with_non_utf8_locale(self) -> None: | ||
| """Unicode paths must survive subprocess decoding under an ASCII locale.""" | ||
| path = "quickstarts/指南.md" | ||
| raw_output = (path + "\0").encode("utf-8") | ||
| check_output = subprocess.check_output | ||
| original_locale = locale.setlocale(locale.LC_CTYPE) | ||
| self.addCleanup(locale.setlocale, locale.LC_CTYPE, original_locale) | ||
| locale.setlocale(locale.LC_CTYPE, "C") | ||
|
|
||
| def git_diff_output(cmd, **kwargs): | ||
| return check_output( | ||
| [sys.executable, "-c", f"import sys; sys.stdout.buffer.write({raw_output!r})"], | ||
| **kwargs, | ||
| ) | ||
|
|
||
| with mock.patch( | ||
| "tools.check_all_links.subprocess.check_output", side_effect=git_diff_output | ||
| ): | ||
| self.assertEqual(get_changed_files_from_git(self.repo_root), [path]) | ||
|
|
There was a problem hiding this comment.
According to the Repository Style Guide (under Internal tooling (tools/)), we do not want to multiply or maintain unit test suites for internal helper scripts, and we should ask to remove any new unit test files or unit test additions submitted for tools/. Please remove these new unit test additions (test_changed_files_preserves_special_characters, test_changed_cli_audits_unicode_filename, and test_changed_files_decodes_utf8_with_non_utf8_locale).
References
- Internal helper scripts under
tools/do not need unit tests, and we do not want to multiply or maintain unit test suites (tools/test_*.py). Never ask authors to add unit tests fortools/changes, and ask to remove any new unit test files or unit test additions submitted fortools/. (link)
| cmd = ["git", "diff", "--name-only", "-z", f"{base}...HEAD"] | ||
| out = subprocess.check_output( | ||
| cmd, cwd=repo_root, encoding="utf-8", stderr=subprocess.DEVNULL | ||
| ) | ||
| lines = [path for path in out.split("\0") if path.endswith((".md", ".ipynb"))] |
There was a problem hiding this comment.
According to the Repository Style Guide (under Internal tooling (tools/)), changes to internal scripts under tools/ are only accepted when they fix a concrete bug actively affecting existing notebooks, Markdown files, or CI checks on the main branch. If this PR only addresses theoretical or synthetic edge cases (such as files with non-ASCII or special characters in their names that do not currently exist on main), we recommend closing this PR. Could you clarify if there are any existing files on main currently affected by this issue?
References
- Changes to internal scripts under
tools/are only accepted when they fix a concrete bug actively affecting existing notebooks, Markdown files, or CI checks on themainbranch. If a PR only addresses theoretical or synthetic edge cases that do not occur onmain, call it out in the review and recommend closing it. (link)
|
Checked this: no file on main (or anywhere in its history) has a name git would quote, and CI's link_checker.yml runs its own git diff instead of --changed, so nothing on main is hit today. Per the tools/ guidelines I'm closing this. Thanks @kkorpal for verifying it earlier. |
check_all_links.py --changedsilently skips filenames that Git quotes, including non-ASCII names. In a temporary Git repository, adding指南.mdwith[Broken](missing.md)makes the current checker report no modified documents and exit 0.Read NUL-delimited paths from
git diff --name-only -zand preserve each filename when filtering documents. Add regressions for Unicode, embedded newlines, tabs, quotes, and leading spaces, including a CLI check that reports the broken link.Validation:
python3 -m unittest tools/test_check_all_links.py -von Python 3.13; both new regressions fail before the fix.git diff --check, and the actual--changedCLI pass.The full offline audit still reports the same two pre-existing broken links in the Weaviate examples as on the base commit.