Code review

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:
 

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

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