Use the maintained git-restore-mtime, and assert it actually restored - #75
Merged
Conversation
The Ubuntu package is git-tools v2022.12, and that release shells out to `git whatchanged`. Current git refuses to run that without `--i-still-use-this`, a flag there is no way to pass through the tool, so it failed twelve times inside one step, restored nothing, and exited 0. Every release since #65 has been a full copy while CI reported success, which the VPS agent measured from the other end as 0 of 3,275 shared inodes. Upstream fixed it: MestreLion/git-tools 91dc541 replaced whatchanged with `git log`, released as v2025.08. chetan/git-restore-mtime-action v2.3 vendors that exact version, verified by reading the vendored script at the pinned SHA -- `__version__ = "2025.08"` and no whatchanged anywhere in it. Taking the action rather than the tarball also drops the apt round trip the step's own comment called out as a thing that can fail on its own, and it pins by SHA like every other action here. The assertion is the half that matters, because the failure mode was a step that exited 0 having done nothing. A restored file cannot be newer than the commit it was dated from, so no file under static/ may be newer than HEAD's own commit time. A checkout necessarily happens after the commit it checks out, so an unrestored tree always breaches that bound and a restored one never does. It calibrates from the repository, so nothing goes stale as content moves. Counting distinct mtime days was written first and is wrong. git restores the LAST COMMIT time and static/ arrived in a bulk import, so a correctly restored tree here has exactly ONE distinct day and that check would have failed the good case. It was caught only because it was run against a real restored clone instead of being trusted, which is the same discipline this whole change exists to enforce. Measured on two clones of this repository, the assertion run verbatim: restored ok margin 651427s unrestored ERROR margin -33196s And the property the deploy actually needs: two independent clones, restored, produce byte-identical path+mtime sets, so --link-dest can match. 1,052 files updated by the new script, the same 1,052 as before. Closes #74. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the deploy workflow to use a maintained git-restore-mtime implementation via a pinned GitHub Action and adds an explicit post-step assertion to ensure static/ mtimes were actually restored, preventing silent “success” that still yields non-linkable releases.
Changes:
- Replaces the Ubuntu
git-restore-mtimepackage install withchetan/git-restore-mtime-actionpinned by commit SHA. - Adds a shell assertion that fails the job if any file under
static/is newer thanHEAD’s commit time (indicating mtimes likely remained at checkout time). - Removes the
apt-getinstallation ofgit-restore-mtime, reducing a network-dependent step.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 9, 2026
ptr727
added a commit
that referenced
this pull request
Aug 9, 2026
* Stop crediting #65 with a fix that never ran The item described the mechanism #75 replaced: installing the Ubuntu package and calling `git restore-mtime static`. CI uses a pinned action now, and make-release.sh requires v2025.08 or newer by either invocation form. The worse half was a claim of mine, added earlier the same day, that release 20260809-030521 was the first production deploy to exercise the restore. It exercised nothing. That release ran v2022.12, which refuses to run under current git and exits 0 regardless, and the host measured the result as 0 of 3,275 shared inodes. The entry asserted a working feature on the strength of a merge rather than a measurement, which is the exact distinction this repo keeps having to relearn. The item now credits #75 and #76, says plainly that #65 shipped broken, and carries what was actually measured: 1052 of 3269 linked on the local production mirror with the tool installed by hand, the release stamped 2026-08-01 rather than at build time, and 584 MB then 18 MB for two releases. It also records the part that will otherwise read as a regression: the first restored release cannot link, because its predecessor was built with unrestored mtimes, so the first deploy after the promotion is expected to link zero and the one after it is the real test. The retained #65 diagnosis keeps its wording and gains a lead-in saying so. It contains a "today" that means 2026-08-08 and names a fix that did not run, both of which read as current state without it. Found by Copilot review on #77. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Use the file's US spelling The lead-in added one commit ago wrote "behaviour" two words away from the "behavior" it was introducing, in a file that uses the US form throughout. Nothing gates spelling here: cspell runs over README.md and HISTORY.md only, so this is convention rather than a rule, which is why it needed a reader to catch it. Found by Copilot review on #78. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Describe the assertion's actual scope, not a stronger one The entry said both paths assert that nothing under static/ is newer than HEAD's commit. CI does. make-release.sh excludes modified and untracked paths, deliberately, because a working tree can legitimately hold a static file newer than any commit and skipping the check whenever the tree is dirty would make it useless during an edit loop. So the claim was stronger than the code, which is the same defect this file keeps producing: prose that describes the rule someone meant rather than the one that runs. It now states the shared invariant as tracked and unmodified files, and names why the two paths differ: a fresh checkout has nothing uncommitted in it, so CI needs no exclusion to reach the same guarantee. Found by Copilot review on #78, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say static/ where static/ is what is walked "CI compares the whole tree" reads as the repository. Both assertions run `find static`, and nothing outside that directory is examined by either, which matters because the sentence is the thing a future edit to the guard would be read against. Verified against both implementations rather than from memory: .github/workflows/deploy-site-task.yml:133 find static -type f deploy/make-release.sh:186 find static -type f The difference between the two paths is the exclusion, not the scope, and the sentence now says so. Found by Copilot review on #78, as a suppressed comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Match the file's number format, and say permissions rather than mode Counts in the new text alternated between 1052 and 1,052 next to 0 of 3,275 in the same block, which makes two measurements of the same quantity look like different quantities at a glance. The comma form is what this file mostly uses, ten instances to six, and 1,052 already appeared with one, so the new text follows it. The preserved 2026-08-08 diagnosis is deliberately left alone. Its lead-in says it is kept as written, and reformatting inside it would make that false for the sake of consistency it explicitly opts out of. And "a badly moded file" reads as fashion rather than permissions. It is now "a file that acquires the wrong permissions", which is what the sentence is about: a hard link carries its inode's mode, so a wrong one rides the chain into every later release. Found by Copilot review on #78, as suppressed comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #74. Takes the maintainer's suggested route — a current upstream tool rather than a new git — and keeps the loud check, because the failure was a step exiting 0 having done nothing.
The tool
The Ubuntu package is git-tools v2022.12, three years stale. That release shells out to
git whatchanged, which current git refuses without--i-still-use-this— a flag there is no way to pass through.Upstream already fixed it:
MestreLion/git-tools91dc541chetan/git-restore-mtime-actionv2.3Verified by reading the vendored script at the pinned SHA rather than trusting the release notes:
__version__ = "2025.08", andwhatchangedappears nowhere in it.Taking the action also drops the
apt-getround trip the step's own comment called out as something that can fail on its own, and it pins by SHA like every other action here.The assertion, which is the half that matters
A restored file cannot be newer than the commit it was dated from, so no file under
static/may be newer than HEAD's own commit time. A checkout necessarily happens after the commit it checks out, so an unrestored tree always breaches that bound and a restored one never does. It calibrates itself from the repository, so no measured constant goes stale as content moves.Run verbatim against two real clones:
The check I wrote first was wrong, and testing is what caught it
My first version counted distinct mtime days, reasoning that a fresh checkout stamps everything in one window. That is true, and the check still fails the good case:
git restores the last commit time, and
static/arrived in a bulk import, so a correctly restored tree here has exactly one distinct day. The restore plainly worked —1,052 files updated, and the oldest mtime moved from the checkout date to the commit date — and my assertion would have reddened every deploy.It was caught only because it was run against a real restored clone instead of being trusted. Which is the same discipline this whole change exists to enforce, arriving immediately and at my expense.
The property the deploy actually needs
Two independent clones, each restored, produce byte-identical path+mtime sets:
1,052 files updated by the new script, the same 1,052 as the original measurement in #65.
What this does not claim
It does not claim the next deploy will hard-link. It claims the mtimes will be restored and that a deploy which fails to restore them will now fail loudly instead of shipping a full copy and reporting success. The shared-inode count is the VPS agent's to measure, and I will tell them once a deploy has genuinely restored rather than once this merges — that distinction is what I got wrong last round.