From fbf48d34be8738d4aa6e88fd049c2f12f4871355 Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Thu, 23 Jul 2026 20:45:17 -0700 Subject: [PATCH] Code review --- README.md | 6 +++--- check-google-java-format.py | 34 +++++++++++++++++++++++++++------- run-google-java-format.py | 32 +++++++++++++++++++++++--------- 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index e4f6baa..e5ee3ed 100644 --- a/README.md +++ b/README.md @@ -461,9 +461,9 @@ the above instructions must be revised. ## Troubleshooting -If you get an error in -```urllib.urlretrieve(gjf_url, gjf_jar_path)``` -then there is a problem with your installation of Python. +If you get an error such as `Problem while retrieving ...` while the script +downloads google-java-format, then there is a problem with your installation of +Python (typically its SSL support). On macOS Sierra, you can correct the problem by running these commands: diff --git a/check-google-java-format.py b/check-google-java-format.py index 077ad5d..8a9b9e5 100755 --- a/check-google-java-format.py +++ b/check-google-java-format.py @@ -15,6 +15,8 @@ # this script can be eliminated, or its interface simplified. import filecmp +import itertools +import os import shutil import stat import subprocess @@ -64,9 +66,26 @@ def under_git(directory: Path, filename: str) -> bool: def urlretrieve(url: str, filename: Path) -> None: - """Like urllib.urlretrieve.""" - with urlopen(url) as in_stream, filename.open("wb") as out_file: - shutil.copyfileobj(in_stream, out_file) + """Like urllib.urlretrieve, but writes `filename` atomically. + + Downloads to a temporary file in the destination directory, then renames it + into place, so an interrupted transfer never leaves a partial file at + `filename`. + """ + with tempfile.NamedTemporaryFile(dir=filename.parent, delete=False) as tmp: + tmp_path = Path(tmp.name) + try: + with urlopen(url) as in_stream: + shutil.copyfileobj(in_stream, tmp) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + # NamedTemporaryFile creates the file mode 0600; instead give it the + # umask-respecting permissions that a normally-created file would have. + umask = os.umask(0o022) + os.umask(umask) + tmp_path.chmod(0o666 & ~umask) + tmp_path.rename(filename) # Don't replace local with remote if local is under version control. @@ -85,15 +104,16 @@ def urlretrieve(url: str, filename: Path) -> None: run_py_path.chmod(run_py_path.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) temp_dir = tempfile.mkdtemp(prefix="check-google-java-format-") +_temp_counter = itertools.count() def temporary_file_name() -> str: - """Return the name of a temporary file. + """Return the name of a temporary file, unique within this run's temp_dir. Returns: the name of a temporary file. """ - return str(Path(temp_dir) / next(tempfile._get_candidate_names())) # ty: ignore[unresolved-attribute] # ruff:ignore[private-member-access] + return str(Path(temp_dir) / f"tmp{next(_temp_counter)}") def cleanup() -> None: @@ -130,8 +150,8 @@ def cleanup() -> None: exit_code = 0 -for i, file in enumerate(files): - if not filecmp.cmp(file, temps[i], shallow=False): +for file, temp in zip(files, temps, strict=True): + if not filecmp.cmp(file, temp, shallow=False): # TODO: gives temporary file name if reading from stdin print("Improper formatting:", file) exit_code = 1 diff --git a/run-google-java-format.py b/run-google-java-format.py index 408a8ec..1f9cb87 100755 --- a/run-google-java-format.py +++ b/run-google-java-format.py @@ -115,9 +115,26 @@ def under_git(directory: Path, filename: str) -> bool: def urlretrieve(url: str, filename: Path) -> None: - """Like urllib.urlretrieve.""" - with urlopen(url) as in_stream, filename.open("wb") as out_file: - shutil.copyfileobj(in_stream, out_file) + """Like urllib.urlretrieve, but writes `filename` atomically. + + Downloads to a temporary file in the destination directory, then renames it + into place, so an interrupted transfer never leaves a partial file at + `filename`. + """ + with tempfile.NamedTemporaryFile(dir=filename.parent, delete=False) as tmp: + tmp_path = Path(tmp.name) + try: + with urlopen(url) as in_stream: + shutil.copyfileobj(in_stream, tmp) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + # NamedTemporaryFile creates the file mode 0600; instead give it the + # umask-respecting permissions that a normally-created file would have. + umask = os.umask(0o022) + os.umask(umask) + tmp_path.chmod(0o666 & ~umask) + tmp_path.rename(filename) # Set gjf_jar_path, or retrieve it if it doesn't appear locally. Does not update @@ -131,13 +148,10 @@ def urlretrieve(url: str, filename: Path) -> None: else: gjf_jar_path = candidate1 # print("retrieving " + gjf_url + " to " + gjf_jar_path) + # urlretrieve downloads atomically (temp file plus rename), which avoids + # races with other concurrent run-google-java-format processes. try: - # Download to a temporary file, then rename atomically. - # This avoids race conditions with other run-google-java-format processes. - # "delete=False" because the file will be renamed. - with tempfile.NamedTemporaryFile(dir=script_dir, delete=False) as f: - urlretrieve(gjf_url, Path(f.name)) - Path(f.name).rename(gjf_jar_path) + urlretrieve(gjf_url, gjf_jar_path) except Exception as e: raise Exception("Problem while retrieving " + gjf_url + " to " + str(gjf_jar_path)) from e