sadpandajoe opened a new pull request, #44582:
URL: https://github.com/apache/superset/pull/44582
### SUMMARY
Small follow-up to #44578, which fixed
`test_committed_template_matches_a_fresh_extraction` flaking under `pytest -n
auto` by extracting from a `git archive` snapshot instead of the live checkout.
That PR was approved and merged with a few non-blocking bot review comments
outstanding; a human reviewer (@rusackas) explicitly called out that the
missing stderr capture was worth a real follow-up ("not a hypothetical"), and
multiple review bots converged on the same spot independently. This PR
addresses those, plus two smaller robustness/doc findings from the same review
thread, in `scripts/translations/check_pot_drift.py`.
Fixed:
- **Capture stderr on both manual `subprocess.Popen` calls** (`git stash
create` in `_archive_ref()`, `git archive` in `extract_fresh()`) and attach it
via `CalledProcessError(..., stderr=...)`, matching what `subprocess.run(...,
capture_output=True, check=True)` already gives the `pybabel extract` call.
Called out by @rusackas, and independently by the bito-code-review and
codeant-ai bots.
- **Fixed an ordering bug in `extract_fresh()`** (bito-code-review): a
failed `git archive` closes stdout early, so `tarfile` raised its own opaque
`ReadError`/`EOFError` *before* the exit code was ever checked — git's real
error was never seen. The tar extraction is now wrapped in `try`/`finally`,
draining stderr and checking the return code in `finally` so a `git archive`
failure raises `CalledProcessError` with git's actual stderr instead of a
confusing tarfile exception.
- **Replaced `assert archive.stdout is not None` with a `RuntimeError`**
(Copilot) — `assert` is stripped under `python -O`, so the guard would silently
vanish in optimized mode.
- **Made the `-o` output path passed to `pybabel extract` absolute**
(`output_path.resolve()`) since `cwd` for that call is now the temp snapshot
dir rather than the repo root. The current caller (`diff()`) already passes an
absolute tempfile path, so this is defense-in-depth for the function's contract
rather than a live bug.
- **Fixed a docstring inaccuracy in `_archive_ref()`** (bito-code-review and
codeant-ai, independently, on the same line): it claimed the tree-ish "matches
the working tree," but `git stash create` only covers tracked files. Corrected
to say it matches the *tracked* working tree, and note a new, not-yet-`git
add`-ed file won't be picked up — this only affects local/pre-commit runs,
since CI always operates on a clean, fully-tracked checkout.
Deliberately NOT changed (also discussed on #44578):
- Copilot's suggestion to harden `tar.extractall()` against path traversal /
malicious archive members. This is `git archive` output of our own trusted
local checkout, not attacker-controlled input — there's already a code comment
addressing exactly this — so adding path-traversal defenses here would be
over-engineering for this context.
- Making `_archive_ref()` include untracked files. `git stash create` was
chosen specifically so the check doesn't touch the working tree while
preserving the existing local pre-commit workflow; changing that would be a
real behavior change, not a follow-up fix. The docstring correction above is
the right-sized fix.
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/scripts/translations/check_pot_drift_test.py -v`
— added tests cover: a failing `git archive` raising `CalledProcessError` with
git's stderr populated (instead of a `tarfile` exception), a failing `git stash
create` raising `CalledProcessError` with stderr populated, the `RuntimeError`
guard when `git archive`'s stdout pipe is missing, and that a relative
`output_path` is resolved to an absolute path before being passed to `pybabel
extract`.
- `ruff check` / `ruff format --check` on both changed files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
