fix: Phase 2 — the confirmed bugs, plus mode-only drift restoration - #23
Merged
Merged
Conversation
`top = 5` reports only the five most severe violations per file and silently discards the rest, so the tool understates how much is wrong -- during the takeover assessment it hid the true severity-3 count (36 reported, 43 actual). The severity-4 gate set is unchanged: handle_metadata carries five violations at that level, which is exactly at the old cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chmod_file interpolated @files into the sprintf *format* string, so a tracked file supplied format directives of its own. A file named '100%n' made --set die after chown_file had run but before chmod_file, touch_file, and extnd_attr -- a half-restored tree with no record of which files were reached. 'a%sb' silently corrupted to 'ab' in the message. --quiet never built the message, which is why the hooks never tripped over it. Every other formatted-output site was audited: chown_file, touch_file, and extnd_attr interpolate into a plain string, and the two remaining sprintf calls already pass their names as arguments. Fixes #17 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
git ls-tree C-quotes any path holding a double quote, a backslash, a control character, or -- under the default core.quotePath -- a byte above 0x7f, so 'héllo.txt' arrived as '"h\303\251llo.txt"'. lstat was handed the quoted string, failed, and the file was skipped: its metadata never saved and never restored, with --quiet hiding even the warning. The ignore list had the same problem. -z removes the quoting rather than asking us to undo it, and NUL is the only separator a filename cannot contain -- a newline is legal in one, so splitting on newlines was never safe either. The ls-tree record is now split on its tab, so a path containing a newline survives too. Restoring such a name needed a second fix. Decoding the note turns git's bytes into characters, and perl hands a character string to chmod and utime as its UTF-8 encoding -- which is not the name on disk. get_note now puts every name back to bytes, so the note stays valid UTF-8 JSON while the filesystem still sees the path git gave us. Fixes #18 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git status --porcelain` puts the staged status in column 1 and the worktree status in column 2, so a staged-only change is "M file" and one changed in both is "MM file". The ignore list matched /^\s\S\s/, which requires a space in column 1, so it saw worktree changes alone: a staged or mixed change was recorded and restored as though the file were not mid-edit. The XY columns are now read by position rather than by regex shape, and a rename or copy has its source path -- the second record git emits under -z -- consumed as a path instead of misparsed as another status line. This also retires the ignore list's grep-with-side-effects, so ControlStructures::ProhibitMutatingListFunctions leaves the perlcritic baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ignore list exists to keep gitperms off a file that is mid-edit, but it was declining the one thing gitperms is for. git tracks the executable bit, so dropping it makes `git status` report the file as modified -- and the file was then skipped, leaving the drift unrepaired. The exemption is narrowed to what it was protecting: a file whose content still matches the blob HEAD records is restored however its mode looks; a file whose content differs is ignored exactly as before, in either status column. A deletion, rename, copy, addition, or unmerged path stays ignored whatever its mode, because none of those is mode-only drift. Content is compared by hashing the file on disk and matching it against HEAD's blob. `git status` cannot answer the question -- it reports "modified" without saying which of content and mode changed -- and `core.fileMode=false` only blinds git to the mode on *disk*, so a mode change already staged in the index still reads as a difference. Hashing is one `git hash-object` per file git reported, and it applies the same filters git would, so a repository using clean/smudge or end-of-line conversion is compared on git's own terms. A file staged with one kind of change and worktree-modified with the other is ignored: content differing anywhere means work in progress. Closes #21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The emptiness of $topdir was checked before GIT_WORK_TREE was exported
and not before the chdir on the next line. git <= 2.11 answers
`rev-parse --show-toplevel` from inside .git with an empty string and
exit 0, and perl <= 5.22 reads chdir('') as chdir() and moves to $HOME --
both inside the range `use 5.014` claims to support. On that pairing the
script did not stop: it resolved every tracked path relative to the home
directory and, under --set, applied chmod, chown, and utime to whatever
sat there.
get_toplevel now refuses an empty answer the same way it refuses an
error, so there is one check rather than two half-applied ones and the
GIT_WORK_TREE guard is no longer needed.
The regression test stages the old answer with a git stub on PATH, since
no current git or perl can reach the pairing on its own.
Fixes #19
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_note recognised a missing note by matching git's own sentence, "No note found for object". git lowercased it between v1.8.0 and v2.43.0 (builtin/notes.c), so the match stopped firing: --create-on-missing silently did nothing, and the friendly guidance a user was meant to see was replaced by a raw error on stderr. Fixing the case would leave the same trap in place -- human-readable output is not an interface. Existence is now asked of `git notes list`, which answers with an exit status: 0 when a note is there, 1 when it is not, and 128 when the object itself will not resolve. The last case is still surfaced as an error rather than reported as a missing note. The suite's TODO-marked scaffolding for --create-on-missing is enabled, and the flag gains a black-box test: it has never once worked end to end. Fixes #13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`open RESTORCON, "|restorecon -f- -v" || warn "..."` parsed as open( RESTORCON, ( "|..." || warn "..." ) ): || binds tighter than the list operator, so the string was simply truthy, it became open's second argument, and the warning was unreachable. close was not checked at all, which is where a piped command's exit status actually surfaces -- so a missing or failing restorecon left no trace whatever. Now a lexical filehandle, the list form of open (no shell), `or warn` for the start failure, and a checked close for the exit status. That clears InputOutput::ProhibitTwoArgOpen and InputOutput::ProhibitBarewordFileHandles from the perlcritic baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The HL test was `$inode != {}`, which compares the address of the record
against that of a fresh anonymous hash: always true. So nlink > 1 alone
was enough to be called a hardlink, and a file whose other link lives
outside the repository -- or is simply untracked -- reached the restore
with a single name in its record. The restore shifts that one name off
and then finds nothing left to relink, warns "No linked files (how did
you get here?!)", and skips the file outright: no mode, no owner, no
timestamps, no attributes.
Read against how the record is built and how set_metadata_in_system
consumes it, the test means "this record already holds another name for
the same inode" -- the only case the restore can act on, since HL exists
to relink every name after the first back to it. That is what it now
checks. A genuine hardlinked pair is still HL: the second name to arrive
finds the record populated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`qx/cat \/selinux\/enforce/` named the original selinuxfs mount point. No current distribution has it -- the filesystem moved under /sys/fs -- so every single run forked a cat that printed a complaint, under --quiet as much as without it, and the answer was always false. The file is now read directly, /sys/fs/selinux/enforce first and the old path second, with a path that will not open simply moving on to the next. Reading the right file is not sufficient on its own: the value has to be stripped before it is believed, because "0\n" is a true string in perl. Left unchomped, an SELinux host that was *not* enforcing would have read as one that was, and restorecon would have run against every restored path. Fixes #7 The issue carries a contributed patch that wraps the old path in a shell conditional; this keeps neither the fork nor the old-path-only lookup, and adds the chomp that patch does not need but this reading does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tags fetch refspec `setup` installs has never worked: its destination reads 'regs/tags/*'. git accepts the config line and then declines the refspec, so a repository set up by this script fetches no tags. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
get_git_dir and get_toplevel printed $? verbatim, so git's exit 128 -- the status for "not a git repository" -- reached the user as "System error: 32768". $? is not an exit code: the exit code is its high byte, and its low byte carries the signal that killed the child instead. One describe_status now turns a wait status into "exited N", "was killed by signal N", or "could not be run" for run_cmd's -1, and the two messages name the git command they came from. The restorecon close reports through it too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Formatting only, no change in behaviour: the ternary in the file-type classification realigns after its condition changed length, and the block-end comments perltidy maintains follow the blocks that moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
'record' reads as either a noun or a verb, which is why perlcritic's NamingConventions::ProhibitAmbiguousNames flags it; the status loop alongside already calls its own record $entry. Below the severity the gate enforces, but it was three violations this branch introduced. Co-Authored-By: Claude Opus 5 <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
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 2: the deferred bug fixes, the three the QA pass turned up, and the #21 design decision. 15 commits, each one compiling and passing the full suite standalone. Suite grew 185 → 200 tests.
Closes #7. Closes #13. Closes #17. Closes #18. Closes #19. Closes #21.
sprintfformat —100%nmade--setexit 255 mid-restorels-tree/status, so non-ASCII and special names stop being silently skippedMMentries now excludedchdir-ing to$HOMErestoreconpipe: lexical handle, list-form open,or warn, checkedclose/sys/fs/selinux/enforce, nocatforksetupwritesrefs/tags, notregs/tagsexited 128, not32768)Three findings worth reading
Fixing #7 naively would have introduced a bug.
"0\n"is truthy in Perl, so once the path is corrected a real SELinux host that is not enforcing reads as enforcing and runsrestoreconover every restored path. The old code was shielded from this only accidentally —catfailed on the nonexistent path and returned"". The value is now chomped. The contributed patch on #7 fixes neither the/sys/fslookup nor this; credit to the reporter for finding the original.#18 needed a second fix to actually work. With save corrected, restore still missed non-ASCII names: decoding the JSON note yields UTF8-flagged strings, and perl hands those to
chmod/utimeas their UTF-8 encoding rather than the bytes on disk, sochmodreturns 0.get_notenow downgrades names back to bytes, keeping the note valid UTF-8 JSON while the filesystem sees git's bytes.The
$inode != {}condition was not a harmless no-op. Being always true, it typed anynlink > 1file as a hardlink — including one whose other link is outside the repo or untracked. At restore that hit theNo linked files (how did you get here?!)guard and the file was skipped entirely: no mode, no owner, no timestamps, no attributes. A whole class of file was silently dropped from every restore.#21 implementation notes
git -c core.fileMode=false diffwas implemented first and failed the staged case —core.fileMode=falseonly blinds git to the mode on disk, so a staged mode change still reads as a difference.hash-objectalso applies the same filters git would, so clean/smudge and EOL repos compare on git's terms.MM→ ignored. Content differing in either column means work in progress; only "content matches HEAD" earns the exemption.--savetoo, not just--set— there is one ignore list. On save the drifted mode is recorded rather than the file being dropped from the note entirely. Flagging it because the decision was framed around restore.Test plan
env -u PERL5LIB perl -c handle_metadata— zero CPAN dependencies still holds./run-testsgreen, 200 tests (was 185)git archiveinto a scratch treegitstub so it is deterministic on any host — the pre-existing probe could not reach the bug on modern git/perl$inode != {}reasoning--saveinheriting the Drift across the executable bit is ignored — the case gitperms exists to repair #21 carve-out is intendedKnown, not fixed
get_metadata_from_systemcomplexity, both from the ignore-list logic. The remedy is extracting that into its own sub — deliberately not done here, since restructuring belongs in its own change rather than smuggled into a bug-fix series.restorecon -f-reads newline-separated paths, so a filename containing a newline still cannot have its SELinux context restored. A limit of restorecon's interface, not of our parsing.t/files are notperltidy-clean (pre-existing); nothing runs perltidy in CI.— agent-authored by Claude