zozo123 commented on PR #73182:
URL: https://github.com/apache/airflow/pull/73182#issuecomment-5782539065
One more commit after the marker fix, and a note on why the guard is shaped
the way it is.
**The extra commit.** `installation-succeeded` is written to
`$GITHUB_OUTPUT` twice and read by nothing. Its only consumer was the
`installation-failed` policy branch you flagged as unreachable, and the
composite action declares no `outputs:` block, so no caller can read it either.
Those four lines go with the branch. The retry loop's `installation_succeeded`
shell variable is a different thing and stays.
**On the guard.** I checked it against the alternatives rather than
assuming. The one worth recording: snapshotting the whole cache tree instead of
prek's marker files would be layout-independent and would need no uncertainty
path at all — which sounds strictly better. It isn't. `prek.log` lives inside
`~/.cache/prek` and is truncated and rewritten on every invocation. Measured
against a real warm cache built from this repo's own config, two consecutive
`install-hooks` runs that repaired nothing: the marker snapshot compares equal
(correct), while a whole-tree path→size snapshot differs in exactly one entry,
`prek.log`, 229191 → 67540 bytes. `tar -C ~ -czf` puts it in the archive, so
every restored cache carries one. A whole-tree snapshot would report a change
on every run and save on every PR run — the opposite of what this PR is for.
Excluding `prek.log` would trade two known prek internals for one, with no gain.
Each conjunct is load-bearing under mutation: removing the guard fails 4 of
the new cases, dropping `any(cache_dir.iterdir())` fails 2, dropping
`cache_dir.is_dir()` fails 3 — on a genuine cache miss the before-snapshot
would raise `FileNotFoundError`, forcing uncertainty and destroying the
`cache-repaired` signal on the most common path. One trap worth flagging for
anyone tempted to shorten it: `any(cache_dir.glob("*"))` is not equivalent,
because `Path.glob` swallows `PermissionError` and returns empty, which fails
*open* on precisely the case this closes.
Local verification: 1392 passed / 1 skipped across `scripts/tests/`, the
gated `RUN_PREK_INTEGRATION=1` integration test passes, and prek static checks
are clean on the changed files.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
--
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]
