(superset) branch master updated: fix(translations): capture stderr and harden check_pot_drift.py follow-ups from #44578 (#44582)

[ru...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 5af034ff6f8 fix(translations): capture stderr and harden check_pot_drift.py follow-ups from #44578 (#44582)
5af034ff6f8 is described below

commit 5af034ff6f8f380c160e797c62bb1cabe8c12c16
Author: Joe Li <jo...@preset.io>
AuthorDate: Mon Sep 28 10:31:33 2026 -0700

    fix(translations): capture stderr and harden check_pot_drift.py follow-ups from #44578 (#44582)
    
    Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
 scripts/translations/check_pot_drift.py            | 52 +++++++++-----
 .../scripts/translations/check_pot_drift_test.py   | 82 ++++++++++++++++++++++
 2 files changed, 117 insertions(+), 17 deletions(-)

diff --git a/scripts/translations/check_pot_drift.py b/scripts/translations/check_pot_drift.py
index 8a26c39855d..0c6afa7a629 100644
--- a/scripts/translations/check_pot_drift.py
+++ b/scripts/translations/check_pot_drift.py
@@ -100,13 +100,16 @@ def _msgid_set(pot_path: Path) -> set[MsgId]:
 
 
 def _archive_ref() -> str:
-    """Return a tree-ish for ``git archive`` that matches the working tree.
+    """Return a tree-ish for ``git archive`` that matches the tracked working tree.
 
     ``git stash create`` builds a commit object for the current index and
     tracked-file modifications without touching the working tree, any ref,
     or the actual stash, so it is safe to call while other processes are
     using this checkout. It prints nothing when there is nothing to stash,
-    so fall back to ``HEAD``.
+    so fall back to ``HEAD``. Because it only knows about tracked files, a
+    new source file with a translatable string that has not yet been
+    ``git add``-ed is not included; that only affects local/pre-commit runs,
+    since CI always operates on a clean, fully-tracked checkout.
 
     Deliberately uses ``Popen`` rather than ``run``: this module's tests
     patch ``subprocess.run`` to fake the single "run pybabel" call, and a
@@ -116,11 +119,14 @@ def _archive_ref() -> str:
         ["git", "stash", "create"],  # noqa: S607
         cwd=ROOT_DIR,
         stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
         text=True,
     )
-    stash_sha, _ = proc.communicate()
+    stash_sha, stash_stderr = proc.communicate()
     if proc.returncode != 0:
-        raise subprocess.CalledProcessError(proc.returncode, proc.args)
+        raise subprocess.CalledProcessError(
+            proc.returncode, proc.args, stderr=stash_stderr
+        )
     return stash_sha.strip() or "HEAD"
 
 
@@ -141,19 +147,31 @@ def extract_fresh(output_path: Path) -> None:
             ["git", "archive", _archive_ref()],  # noqa: S607
             cwd=ROOT_DIR,
             stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
         )
-        assert archive.stdout is not None
-        with tarfile.open(fileobj=archive.stdout, mode="r|") as tar:
-            # Use the safe extraction filter (PEP 706) where available; older
-            # Python patch releases without the backport still work, just
-            # without that defense-in-depth (this is our own trusted `git
-            # archive` output, not attacker-controlled).
-            data_filter = getattr(tarfile, "data_filter", None)
-            if data_filter is not None:
-                tar.extraction_filter = data_filter
-            tar.extractall(snapshot_dir)  # noqa: S202 (own trusted `git archive` output)
-        if archive.wait() != 0:
-            raise subprocess.CalledProcessError(archive.returncode, archive.args)
+        if archive.stdout is None:
+            raise RuntimeError("git archive did not open a stdout pipe")
+        try:
+            with tarfile.open(fileobj=archive.stdout, mode="r|") as tar:
+                # Use the safe extraction filter (PEP 706) where available; older
+                # Python patch releases without the backport still work, just
+                # without that defense-in-depth (this is our own trusted `git
+                # archive` output, not attacker-controlled).
+                data_filter = getattr(tarfile, "data_filter", None)
+                if data_filter is not None:
+                    tar.extraction_filter = data_filter
+                tar.extractall(snapshot_dir)  # noqa: S202 (own trusted `git archive` output)
+        finally:
+            # A failed `git archive` closes stdout early, so `tarfile` raises
+            # its own opaque `ReadError`/`EOFError` before we get a chance to
+            # look at git's actual error; draining stderr and the return code
+            # here, and raising from `finally`, surfaces that real cause
+            # instead of (and in place of) the tarfile exception.
+            _, archive_stderr = archive.communicate()
+            if archive.returncode != 0:
+                raise subprocess.CalledProcessError(
+                    archive.returncode, archive.args, stderr=archive_stderr
+                )
 
         subprocess.run(  # noqa: S603
             [
@@ -162,7 +180,7 @@ def extract_fresh(output_path: Path) -> None:
                 "-F",
                 str(snapshot_dir / "superset" / "translations" / "babel.cfg"),
                 "-o",
-                str(output_path),
+                str(output_path.resolve()),
             ]
             + EXTRACT_FLAGS,
             cwd=snapshot_dir,
diff --git a/tests/unit_tests/scripts/translations/check_pot_drift_test.py b/tests/unit_tests/scripts/translations/check_pot_drift_test.py
index b7943b36ca4..f4ad485d4c1 100644
--- a/tests/unit_tests/scripts/translations/check_pot_drift_test.py
+++ b/tests/unit_tests/scripts/translations/check_pot_drift_test.py
@@ -22,6 +22,8 @@ its on-disk path, matching ``check_translation_regression_test.py``.
 """
 
 import importlib.util
+import io
+import subprocess
 from pathlib import Path
 from unittest.mock import MagicMock, patch
 
@@ -141,6 +143,86 @@ def test_main_exits_one_and_lists_drift_when_out_of_sync(
     assert "babel_update.sh" in out
 
 
+class _FakeArchiveProcess:
+    """Minimal stand-in for the ``git archive`` ``Popen`` object."""
+
+    def __init__(self, stdout: bytes, stderr: bytes, returncode: int) -> None:
+        self.stdout = io.BytesIO(stdout)
+        self.args = ["git", "archive", "HEAD"]
+        self.returncode = returncode
+        self._stderr = stderr
+
+    def communicate(self) -> tuple[bytes, bytes]:
+        return b"", self._stderr
+
+
+def test_archive_ref_raises_called_process_error_with_stderr_on_stash_failure() -> None:
+    fake_stderr = "fatal: not a git repository\n"
+    fake_process = MagicMock()
+    fake_process.communicate.return_value = ("", fake_stderr)
+    fake_process.returncode = 1
+    fake_process.args = ["git", "stash", "create"]
+
+    with patch.object(check_pot_drift.subprocess, "Popen", return_value=fake_process):
+        with pytest.raises(subprocess.CalledProcessError) as exc_info:
+            check_pot_drift._archive_ref()
+
+    assert exc_info.value.stderr == fake_stderr
+
+
+def test_extract_fresh_raises_called_process_error_with_stderr_on_archive_failure(
+    tmp_path: Path,
+) -> None:
+    # A truncated/empty tar stream makes `tarfile` raise its own `ReadError`
+    # before `extract_fresh` gets a chance to look at git's exit code; the
+    # fix must surface git's stderr via `CalledProcessError` instead.
+    fake_stderr = b"fatal: your current branch does not have any commits yet\n"
+    fake_process = _FakeArchiveProcess(stdout=b"", stderr=fake_stderr, returncode=128)
+
+    with (
+        patch.object(check_pot_drift, "_archive_ref", return_value="HEAD"),
+        patch.object(check_pot_drift.subprocess, "Popen", return_value=fake_process),
+    ):
+        with pytest.raises(subprocess.CalledProcessError) as exc_info:
+            check_pot_drift.extract_fresh(tmp_path / "fresh.pot")
+
+    assert exc_info.value.stderr == fake_stderr
+
+
+def test_extract_fresh_raises_runtime_error_when_archive_has_no_stdout_pipe(
+    tmp_path: Path,
+) -> None:
+    fake_process = MagicMock(stdout=None)
+
+    with (
+        patch.object(check_pot_drift, "_archive_ref", return_value="HEAD"),
+        patch.object(check_pot_drift.subprocess, "Popen", return_value=fake_process),
+    ):
+        with pytest.raises(RuntimeError, match="stdout"):
+            check_pot_drift.extract_fresh(tmp_path / "fresh.pot")
+
+
+def test_extract_fresh_passes_an_absolute_output_path_to_pybabel(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    # `cwd` for the `pybabel extract` call is the temp snapshot directory, not
+    # the caller's cwd, so a relative `output_path` must be resolved before
+    # being passed on the command line.
+    monkeypatch.chdir(tmp_path)
+    captured_args: list[str] = []
+
+    def fake_run(args: list[str], **_kwargs: object) -> MagicMock:
+        captured_args.extend(args)
+        return MagicMock(returncode=0)
+
+    with patch.object(check_pot_drift.subprocess, "run", side_effect=fake_run):
+        check_pot_drift.extract_fresh(Path("fresh.pot"))
+
+    output_arg = captured_args[captured_args.index("-o") + 1]
+    assert Path(output_arg).is_absolute()
+    assert output_arg == str((tmp_path / "fresh.pot").resolve())
+
+
 def test_committed_template_matches_a_fresh_extraction() -> None:
     """The actual regression test: source and the committed template agree.