Skip to content

Restore mtimes in the local release too, so both paths fail the same way - #76

Merged
ptr727 merged 4 commits into
developfrom
local-restore-mtime
Aug 9, 2026
Merged

Restore mtimes in the local release too, so both paths fail the same way#76
ptr727 merged 4 commits into
developfrom
local-restore-mtime

Conversation

@ptr727

@ptr727 ptr727 commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Follow-on to #75, at the maintainer's request: the local release path should mimic what CI does.

The gap, and why it was invisible

deploy/make-release.sh restored nothing and asserted nothing. That went unnoticed because this host's working tree is long-lived, so static/ already carries old mtimes and local releases were linking 1052 files while CI linked zero.

A fresh clone here — a clean-room build, a second checkout, a new machine — would have reproduced the CI defect exactly, silently, and cost a full copy per release.

What changed

git-restore-mtime is now required, not optional. Absent, the release refuses and names the version to install. Skipping when a tool is missing is precisely how the CI version shipped broken for four releases.

The assertion is the same self-calibrating one the workflow uses — nothing under static/ may be newer than HEAD's commit — with one difference CI does not need: locally modified and untracked paths are excluded. 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. A clean tree takes the same single find the workflow runs.

Demonstrated failing before being trusted

1. not installed
   git-restore-mtime not found on PATH
     it is what makes --link-dest able to link, and a release built without it is a full copy
     install git-tools v2025.08 or newer -- the v2022.12 in Debian and Ubuntu calls
     'git whatchanged', which current git refuses to run, so it restores nothing and exits 0
   exit=1

2. installed but silently doing nothing   <- the real v2022.12 bug, reproduced with a stub
   ==> restoring file mtimes
     1,052 files to be processed in work dir
   mtime restore did nothing: static/ holds unmodified files newer than HEAD's commit,
     so they still carry their checkout time and --link-dest will link nothing
   exit=1

3. working v2025.08
   1,052 files updated
   ==> mtimes restored, newest 1785594994 against HEAD 1786280126

Case 2 is the one that matters: the stub prints the same reassuring line the broken tool prints, and the gate catches it anyway.

The finding worth reading, which is not a defect

The first restored release cannot link, by construction. It is compared against a predecessor built with unrestored mtimes, so nothing matches and the existing zero-shared-files guard fires and refuses the release.

That guard is correct. The changeover needs one NO_LINK_DEST=1 release to seed a restored generation — which is exactly what that knob exists for. Measured on the staging mirror:

Release Result
first restored, against an unrestored predecessor 0 of 3269 — guard refuses
seeded with NO_LINK_DEST=1 full copy, installs
the next ordinary release 1052 of 3269 files hard-linked

1052 is the same number the two-clone measurement in #65 predicted, and the same count Hugo reports as static files. The mirror answers PASS - 1253 URLs honored afterwards.

This applies to the VPS too, and it corrects what I told the host side in §R. Their next deploy will link 0, not ~1052, because it compares against 20260809-030521 which was built with unrestored mtimes. The deploy has no equivalent guard so it will simply proceed; the deploy after that is the one where the shared-inode count should jump. I am telling them so they do not read the expected zero as the fix having failed again.

The deploy workflow restores mtimes and asserts the restore happened.
make-release.sh did neither, and the gap was invisible on this host
because a long-lived working tree already carries old mtimes, so local
releases linked 1052 files while CI linked zero. A fresh clone here would
have reproduced the CI defect exactly and said nothing.

git-restore-mtime is required rather than optional. Absent, the release
refuses to build and names the version to install, because skipping when a
tool is missing is how the CI version shipped broken for four releases: it
printed a reassuring line and restored nothing.

The assertion is the same self-calibrating one the workflow uses, with one
difference that CI does not need. A working tree can legitimately hold a
static file newer than any commit, so locally modified and untracked paths
are excluded rather than the check being skipped whenever the tree is
dirty. A clean tree takes the same single find the workflow runs.

Demonstrated failing before being trusted, all three states:

  not installed          exits 1, names the version and the reason
  installed but a no-op  exits 1 on the assertion, having printed the same
                         "1,052 files to be processed" line the broken
                         v2022.12 prints
  working v2025.08       1,052 files updated, assertion passes

The second is the real bug reproduced with a stub, rather than a
hypothetical.

One thing this surfaced that is not a defect and needs saying. The first
restored release CANNOT link, because it is compared against a predecessor
built with unrestored mtimes, so the existing zero-shared-files guard
fires and refuses it. That guard is correct and the changeover needs one
NO_LINK_DEST=1 release to seed a restored generation, which is what that
knob already exists for. Measured on the staging mirror:

  first restored release, against an unrestored predecessor      0 of 3269
  seeded with NO_LINK_DEST=1                                     full copy
  the next ordinary release                                   1052 of 3269

1052 is the same number the two-clone measurement in #65 predicted and the
same count Hugo reports as static files. The mirror still answers
PASS - 1253 URLs honored afterwards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 9, 2026 13:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the local release script to restore static/ file mtimes and enforce the same “restore actually happened” assertion used in CI, so local releases fail loudly (instead of silently producing full-copy releases) when mtime restoration is missing or ineffective.

Changes:

  • Require an mtime-restore tool before building a release, instead of allowing a silent fallback.
  • Restore mtimes for static/ and assert no clean static/ file is newer than HEAD’s commit time (with local exclusions for modified/untracked paths).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread deploy/make-release.sh Outdated
Requiring `git-restore-mtime` on PATH would have refused a correctly
installed tool. The Debian and Ubuntu package puts it in git's exec-path
at /usr/lib/git-core, where only the subcommand form resolves, which is
what the deploy workflow's own comment says and what this script ignored.
A manual install to /usr/local/bin gives the opposite: the bare name works
and the subcommand does not. Both are now accepted, and the one that
resolves is the one used.

More usefully, the version is gated rather than left to the assertion.
v2022.12 fails in the one way an outcome check catches late and a reader
never catches at all: it calls `git whatchanged`, current git refuses to
run that, so it prints files to be processed, processes none, and exits 0.
Refusing it here names the cause, where the assertion can only report the
symptom. 2025.08 is the floor because that is the release which replaced
whatchanged with `git log`.

Versions are YYYY.MM, so dropping the dot compares them as integers.

Four states, each demonstrated rather than assumed:

  absent            names both invocation forms and where to get it
  v2022.12          refused, with the whatchanged defect named
  bare name         restores, 1052 of 3269 linked
  git subcommand    restores, 1052 of 3269 linked

The last was tested through GIT_EXEC_PATH against a directory carrying the
real exec-path plus the script, so the bare name genuinely did not resolve
and only the subcommand branch could have run.

Mirror still answers PASS - 1253 URLs honored.

Found by Copilot review on #76.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 9, 2026 13:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

deploy/make-release.sh:121

  • With set -euo pipefail, this command substitution will exit the script immediately if the grep finds no match (exit=1), so the intended if [ -z "$mtime_version" ] error message never runs. This can turn a version-format change (or unexpected output) into a silent hard exit without the helpful diagnosis.
mtime_version="$("${MTIME_CMD[@]}" --version 2>/dev/null | grep -oE '[0-9]{4}\.[0-9]{2}' | head -1)"

deploy/make-release.sh:153

  • The git status --porcelain -z output is not always one NUL-separated record per path (e.g., renames/copies include two paths), so stripping a fixed 3-byte prefix (${entry:3}) can mis-populate mtime_dirty. That can make the exclusion set incomplete and cause the mtime assertion to fail even though the only “newer than HEAD” files are part of a rename/copy that should have been excluded as uncommitted.
declare -A mtime_dirty=()
while IFS= read -r -d '' entry; do
	mtime_dirty["${entry:3}"]=1
done < <(git status --porcelain -z -- static)

Two defects in the code added one commit ago, both of which made a guard
describe a case it could not reach.

With `set -e` and `pipefail`, an unmatched grep inside a command
substitution aborts the script at the assignment, so the "did not report a
version" branch below it was unreachable and an unparseable version would
have surfaced as a bare exit 1 with no message. Reproduced in isolation
first: the line after the assignment never printed. Tolerating the failed
match on that assignment makes the diagnostic run, verified with a stub
reporting "version unknown".

And `git status --porcelain -z` emits TWO NUL records for a rename or a
copy, `XY <new>` then a bare `<old>`. The loop read the second as another
status record and stripped three characters off a bare path, recording
`tic/a.txt` for `static/a.txt`. The real path then stayed out of the
exclusion set, so the assertion could fail on a file that is legitimately
uncommitted. Both halves are now excluded, since both are uncommitted.

Measured against a real rename in static/ rather than a constructed one:

  R  static/apple-touch-icon-renamed.png
  static/apple-touch-icon.png

  excluded: static/apple-touch-icon-renamed.png
  excluded: static/apple-touch-icon.png
  ==> 2 uncommitted path(s) under static/, excluded from the mtime check
  1051 of 3269 files hard-linked

One fewer than 1052, which is the renamed file correctly not matching. The
rename was reverted afterwards and the mirror rebuilt from the clean tree,
answering PASS - 1253 URLs honored.

Both found by Copilot review on #76, as suppressed comments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 9, 2026 13:20
@ptr727

ptr727 commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Both fixed in 69b1cad. Both were in code I added one commit earlier, and both made a guard describe a case it could not reach.

1. The version diagnostic was unreachable

With set -euo pipefail, this command substitution will exit the script immediately if the grep finds no match.

Reproduced in isolation before touching anything:

before
exit=1          <- "after:" never printed, the diagnostic never ran

So an unparseable version would have surfaced as a bare exit 1 with no message — the same silent shape this whole change exists to eliminate, in the guard meant to eliminate it. The assignment now tolerates the failed match, verified with a stub reporting version unknown:

git-restore-mtime did not report a version, so it cannot be checked for the whatchanged defect
exit=1

2. A rename emits two records

The git status --porcelain -z output is not always one NUL-separated record per path.

Correct. Confirmed against a real rename in static/:

R  static/apple-touch-icon-renamed.png
static/apple-touch-icon.png

The loop read the second as another status record and stripped three characters off a bare path — recording tic/a.txt for static/a.txt. The real path then stayed out of the exclusion set, so the assertion could fail on a file that is legitimately uncommitted. Exactly the failure you described.

Both halves are now excluded, since both are uncommitted:

excluded: static/apple-touch-icon-renamed.png
excluded: static/apple-touch-icon.png
==> 2 uncommitted path(s) under static/, excluded from the mtime check
1051 of 3269 files hard-linked

1051 rather than 1052 — the renamed file correctly not matching the previous release. The rename was reverted afterwards and the mirror rebuilt from the clean tree, answering PASS - 1253 URLs honored.

Tested with an actual git mv rather than a hand-written porcelain string, because the thing in doubt was what git emits.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

deploy/make-release.sh:134

  • If both invocation forms are present, the script always prefers git-restore-mtime on PATH. That means a stale PATH install (e.g., v2022.12) will cause an early failure even when git restore-mtime is also available and meets MTIME_MIN. To better match the intent of “accept both forms”, consider falling back to the other form when the initially selected command is below MTIME_MIN.
if [ "${mtime_version//./}" -lt "${MTIME_MIN//./}" ]; then
	echo "${MTIME_CMD[*]} is $mtime_version, and $MTIME_MIN or newer is required" >&2
	echo "  before $MTIME_MIN it calls 'git whatchanged', which current git refuses to run, so it" >&2
	echo "  restores nothing and still exits 0 -- every release would silently be a full copy" >&2
	exit 1
fi

Both invocation forms were accepted and the bare name always won, so a
stale manual install at /usr/local/bin vetoed a current packaged one
behind it and the release refused to build with a perfectly good tool
present. Accepting both forms and then letting the worse one decide is
not really accepting both.

Each candidate is now version-checked and the first ACCEPTABLE one wins.
Only when none meets the floor does it refuse, and it names what it found
rather than only what it wanted.

Measured, with a 2022.12 stub on PATH and a real 2025.08 in git's
exec-path:

  ==> restoring file mtimes with git restore-mtime 2025.08
  ==> 1052 of 3269 files hard-linked

and with only the stale one reachable:

  no usable git-restore-mtime: found git-restore-mtime 2022.12,
  git restore-mtime 2022.12, and 2025.08 or newer is required

Both forms report the same tool there, correctly: git resolves a
subcommand from PATH as well as from its exec-path, so one stale binary
is genuinely both candidates.

Found by Copilot review on #76, as a suppressed comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 9, 2026 13:27
@ptr727

ptr727 commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Fixed in f5330b4. Correct, and the phrasing of the finding is the useful part: accepting both forms and then letting the worse one decide is not really accepting both.

A stale manual install at /usr/local/bin would veto a current packaged one behind it, and the release would refuse to build with a perfectly good tool present.

Each candidate is now version-checked and the first acceptable one wins, rather than the first that merely exists. It refuses only when none meets the floor, and names what it found instead of only what it wanted.

Measured with a 2022.12 stub on PATH and a real 2025.08 in git's exec-path:

==> restoring file mtimes with git restore-mtime 2025.08
==> 1052 of 3269 files hard-linked

And with only the stale one reachable:

no usable git-restore-mtime: found git-restore-mtime 2022.12, git restore-mtime 2022.12,
and 2025.08 or newer is required

One detail worth recording from that second run: both forms report the same tool, correctly. git resolves a subcommand from PATH as well as from its exec-path, so a single stale binary genuinely is both candidates. The message says so rather than implying two separate installs.

Mirror still answers PASS - 1253 URLs honored.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit b30d62c into develop Aug 9, 2026
5 checks passed
@ptr727
ptr727 deleted the local-restore-mtime branch August 9, 2026 13:35
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants