refactor!: modernize gitperms onto zero CPAN dependencies - #15
Merged
Merged
Conversation
The modernization targets core Perl 5.14+ (JSON::PP became core there), so raise the version floor from 5.006. Switch the shebang to /usr/bin/env perl so an installed hook picks up a version-managed perl rather than only the system one. Co-Authored-By: Claude Opus 5 <[email protected]>
Nothing in the repository references ReadKey, ReadMode, ReadLine, or any other Term::ReadKey export; the import has been dead since the interactive missing-note prompt was removed in 2013. Co-Authored-By: Claude Opus 5 <[email protected]>
Route all five call sites through one run_cmd helper built on IPC::Open3, IO::Select, and Symbol::gensym -- all core. It keeps the ( stdout, stderr, $? ) shape and the trailing-newline chomp the previous module provided, so the callers are unchanged apart from the list-of-arguments calling convention and splitting the two array-collecting sites themselves. gensym is what keeps stderr on its own pipe: open3 merges the child's stderr into stdout when the third argument is false. Both pipes are then read through IO::Select rather than one after the other -- draining stdout to EOF first deadlocks as soon as stderr fills its 64K pipe buffer, which a repository-sized `git ls-tree` reaches easily. Co-Authored-By: Claude Opus 5 <[email protected]>
Mechanical only, no behaviour change. The file predates the repo's .perltidyrc, so the first run touches long-standing regions (ternary alignment, whitespace, closing-side comments) as well as the newly added code. Kept as its own commit so the dependency-replacement diffs stay readable. Co-Authored-By: Claude Opus 5 <[email protected]>
GIT_DIR was derived as abs_path(dirname($0).'/../'), which hardcodes an install at .git/hooks/. That is wrong in a linked worktree (.git is a file, and the real git dir lives under the main checkout), in a submodule, with a separate git dir, and for any copy on PATH. GIT_WORK_TREE came from core.worktree, which is normally unset, so it was exported as the empty string on every run. Derive both from git itself -- rev-parse --git-dir, made absolute because git answers relatively whenever it can and the caller chdirs -- and export GIT_WORK_TREE only when there is a top level to point at. Both are resolved before either is exported: with GIT_DIR set and no work tree, git would answer --show-toplevel with the current directory. Verified from a worktree root, a subdirectory, the main checkout, and with a relative GIT_DIR in the environment as git sets it for hooks. Co-Authored-By: Claude Opus 5 <[email protected]>
Linux::Ext2::Attributes cannot be built on a modern system at all: the kernel header it needs, <linux/ext2_fs.h>, was dropped from the Linux UAPI, so its generated constants module never compiles. Call the two ioctls directly instead -- ioctl is a perl builtin and needs nothing from CPAN. The ioctl numbers encode the size of their argument, so hardcoding the familiar 0x80086601/0x40086602 would silently address the wrong ioctl on a 32-bit build. They are derived from the running perl's sizeof(long) via pack 'L!' and the asm-generic/ioctl.h layout instead. strip() and set_attrs() are reproduced rather than approximated: the module masked out a fixed set of bits the kernel reports but refuses to accept back (dirty, nocomp, ecompr, index, huge_file, extents), and on restore it kept those bits as the file already had them. FS_READONLY_FLAGS is that same set, named from linux/fs.h. Files are opened O_RDONLY|O_NONBLOCK, as lsattr(1) does, so a fifo in the tree cannot stall the run waiting for a writer -- the previous plain open would have blocked. Verified against lsattr: reads agree on a file, a directory, and /tmp's hash-indexed directory; a set/clear round trip of the nodump and noatime flags leaves the extents bit untouched; fifos and /dev/null report ENOTTY without blocking. Co-Authored-By: Claude Opus 5 <[email protected]>
The one call used no_create => 1, which is exactly what utime does: it sets the times of files that exist and does not create the ones that don't. Both return the count of files changed, so the caller's check is unchanged. Verified that utime leaves a missing file uncreated and returns 0 for it. Co-Authored-By: Claude Opus 5 <[email protected]>
Fifos now go through POSIX::mkfifo, which is core and needs no privilege; character and block devices shell out to mknod(1), which is the root-only path the previous code was too. The mknod(1) call goes through run_cmd rather than qx: the argument is a path out of the repository, and qx would hand it to a shell. That also means a failure reports mknod's own message instead of a bare errno. perl's stat reports rdev in glibc's dev_t encoding, which splits the major and minor numbers across the value rather than storing them contiguously, so the split follows sys/sysmacros.h. Verified against stat(1) on /dev/null, /dev/zero, /dev/sda, /dev/tty, and /dev/loop0. POSIX is imported for mkfifo alone rather than wholesale; nothing else in the script used any of its other 586 exports, and the wide import was silently re-exporting Fcntl's S_IS* and O_* constants over the explicit ones. Co-Authored-By: Claude Opus 5 <[email protected]>
YAML::Syck is the last CPAN dependency; JSON::PP has been core since 5.13.9, below this script's 5.14 floor. Both sides of the note move at once: Dump becomes an encode in set_note, Load becomes a decode in get_note. BREAKING CHANGE: notes written by an earlier gitperms are YAML and will not decode. There is no migration path and none is intended -- re-run --save --force to rewrite the note for a commit. canonical sorts the keys so consecutive notes diff cleanly, and pretty keeps `git notes show` readable. utf8 on both sides keeps a non-ASCII path round-tripping as bytes, matching how the note is written to and read back from git. The note file is now closed before git reads it. tempfile's buffer was previously left unflushed while `git notes add -F` read the same path. Co-Authored-By: Claude Opus 5 <[email protected]>
Phase 1 replaced every CPAN dependency with core perl and changed the note
format, with nothing verifying any of it. This adds the host, unprivileged
tier -- 185 assertions across the areas where that work could plausibly be
wrong -- plus the entry point and tier structure the remaining two tiers slot
into without rearranging anything.
run-tests selects tiers from what the host provides; a tier whose
prerequisites are missing reports SKIP, never FAIL, so a
contributor with only perl still gets a useful result
t/*.t tier 1: compile and zero-dependency invariant, run_cmd,
the JSON note, the ext2 attribute ioctls, path
derivation, option handling, metadata collection, and
the ignore list
t/lib/...Harness compiles handle_metadata's own source -- prelude plus
subroutines, minus the top-level body -- into a test
package, since the script cannot be required. Both split
points are asserted, so a restructure fails loudly
rather than silently testing nothing.
t/lib/...Repo throwaway repositories with the ambient git config
neutralised, a black-box runner, and a forked
wall-clock timeout so a reintroduced deadlock fails
instead of hanging the suite
t/tier2, t/tier3 runners with their prerequisites and the overlayfs trap
documented; both report SKIP until they carry tests
Verified green on perl 5.22, 5.26, 5.30, 5.34, 5.38 and 5.42. perl:5.22 is
the floor the matrix can reach: perl:5.14 through perl:5.20 still exist as
tags but ship schema-1 manifests that modern docker refuses to pull.
Tests that record wrong-but-current behaviour are marked TODO with the reason,
so the suite stays green while documenting what Phase 2 has to flip.
Co-Authored-By: Claude Opus 5 <[email protected]>
Two jobs, producing exactly two check contexts:
perlcritic
tier1
Those strings are what issue #10 needs for required_status_checks. Tier 2 and
tier 3 get no job: their runners exist but carry no tests, and a job that
always passes is worse than no job when a ruleset may come to require it.
perlcritic is gated differentially rather than absolutely. handle_metadata
carries five severity-4-and-above violations, every one of them a bug that is
deliberately still present -- the mutating grep in the ignore-list parser, the
two-arg bareword open in the SELinux path, and the GIT_DIR/GIT_WORK_TREE
assignments. An absolute gate would be red from the first run, which would
either wedge the branch ruleset or teach everyone to ignore the check. So
t/perlcritic.sh compares the current violation set against
t/perlcritic.baseline: a new violation fails, and a baseline entry that stops
firing also fails so the file cannot rot. Each entry carries its reason.
perlcritic runs from ghcr.io/harleypig/perlcritic:1.156 -- the operator's own
image, so the job neither builds Perl::Critic nor pins a CPAN version here. It
is invoked with docker run rather than as a job container, because a job
container has to carry Node.js for actions/checkout and this image does not.
There is no :latest tag, so the version is explicit.
actions/checkout is pinned to v7, the current major.
Co-Authored-By: Claude Opus 5 <[email protected]>
The branch-protection section said the repo has no CI. It does now, so name the two contexts issue #10 needs and state that tiers 2 and 3 contribute none until they carry tests. Co-Authored-By: Claude Opus 5 <[email protected]>
Owner
Author
Tier 1 tests added — 185 assertions, greenPushed three commits adding the test harness, tier 1, and CI. The suite passes; tiers 2 and 3 are scaffolded and report SKIP, not FAIL, so the next increment slots in without restructuring. The author's claims were independently verifiedEach of these was re-derived rather than taken on trust:
Bugs found — all pre-existing, none introduced by this PRFiled, not fixed, to keep this PR reviewable:
Two decisions needed1. Perl floor. The suite is verified green on 5.22, 5.26, 5.30, 5.34, 5.38, 5.42. It cannot reach lower: the 2. perlcritic gate shape. CI check contexts, for #10Both are job names. Tiers 2 and 3 have no job, so no context — #10 must not require one for them, or the ruleset wedges. Left for the next incrementTier 2 wiring (matrix already validated manually); tier 3 (chown to arbitrary uids, device nodes, — agent-authored by Claude |
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
1 participant
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.
Summary
Phase 1 of the takeover: replace every CPAN dependency with core Perl, and stop deriving the repository paths from the script's own location.
env -u PERL5LIB perl -c handle_metadatanow succeeds with nothing installed. Before this branch it died atFile::Touch. The newest module remaining isJSON::PP(first in core at 5.13.9), comfortably under the newuse 5.014floor.Linux::Ext2::AttributesioctlIPC::Run3::SimpleIPC::Open3File::TouchutimebuiltinUnix::Mknodmknod(1)+POSIX::mkfifoYAML::SyckJSON::PPTerm::ReadKeyLinux::Ext2::Attributeswas the blocker: it cannot build on current systems because<linux/ext2_fs.h>was removed from the Linux UAPI, so gitperms has been dead on arrival for years. Coreioctlreplaces it using the sameFS_IOC_GETFLAGS/FS_IOC_SETFLAGSinterfacechattritself uses.BREAKING: the note format is now JSON
Notes written by the previous version are not readable by this one, and there is no migration path. This is deliberate. The pre-takeover code is preserved on
legacy.Location independence
GIT_DIRwas derived fromdirname($0).'/../', hardcoding "I live in.git/hooks/". That broke worktrees (where.gitis a file), submodules, separate git-dirs, and any install onPATH. It now asks git viarev-parse. This is also the prerequisite for the planned pre-commit integration.Implementation notes worth reviewing
_IOR('f',1,long)depends onsizeof(long)— the 64-bit0x80086601would be wrong on 32-bit. Computed fromlength pack 'L!', 0.strip()/set_attrs()semantics were reproduced from the original tarball, not guessed — includingEXT2_READONLY_FLAGS(0xC1D00), spelled out as six named constants. Verified againstlsattr.O_RDONLY|O_NONBLOCK(aslsattr(1)does). A plainopen '<'would block forever on a fifo waiting for a writer.gensymis load-bearing inopen3— a false third argument silently merges stderr into stdout. Both pipes are drained viaIO::Select; reading stdout to EOF first deadlocks once stderr fills its 64 KB buffer. Tested with 900 KB on both streams.mknodgoes through the helper, notqx— the argument is a repository path andqxwould hand it to a shell.Scope
handle_metadataonly.setupand the hooks are untouched. The Phase 2 bug fixes are deliberately excluded so the dependency diff stays reviewable; perlcritic at severity 4 is 6 before, 6 after — the identical six, all pre-existing and all on the Phase 2 list.The
perltidyreflow of long-standing regions is isolated in its ownstyle:commit.Found while working, filed not fixed
--create-on-missingis dead. git lowercasedno note found for object, so the regex never matches. Verified on 2.43.0. Real functional break, not previously known.long-width where the kernel exchanges anint. Harmless on little-endian; on hold.Test plan
env -u PERL5LIB perl -c handle_metadatasucceeds with no CPAN modules installedModule::CoreList, newest is 5.13.9 < 5.014 floorperlcritic --severity 4shows no regression (6 → 6)lsattr; set/clear round trip preserves the extents bitGIT_DIR, and outside a repoPATH— neither possible beforeIPC::Open3helper's stderr handling— agent-authored by Claude