Repository navigation
fix(ci): check the exact lock tree entry and clean up lockfix test dirs - #230
Merged
Merged
Conversation
lockfix-apply.sh used `ls-files -s`, a pathspec match, so a directory committed at a lock path passed the 100644 check through its child. `ls-tree HEAD` reads the entry itself. The tests now keep their scratch repos under one temp root removed in after(), use fileURLToPath for the symlink target, and build the 100755 case with chmodSync plus the commit helper. A new test covers a directory at the lock path. Refs #224
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.
Follow-up to #224. These fixes come from a code-quality review of mforce/cluckwork#1374, which ports part of this lockfix to cluckwork. I checked each finding against collectify's
main, and these three apply here too.Changes
lockfix-apply.shchecked a lock's mode withgit ls-files -s -- "$rel", which matches a path prefix. A directory committed at the lock path, holding one regular file, passed as100644. The classifier refused that case later, so it was not a hole. The check now readsgit ls-tree HEAD -- "$rel"and requires exactly100644 blob.lockfix.test.mjscreated scratch directories withmkdtempSyncand never removed them. They now share one scratch root, which anafter()hook deletes.fileURLToPathinstead ofnew URL(...).pathname, which breaks on a path with a space. The mode test now says it tests100755, and it sets the mode withchmodSyncand the existingcommithelper.Verification
node --test .github/scripts/lockfix.test.mjs .github/scripts/lockfix-stage.test.mjspasses 24 of 24. A new test commits a directory at the lock path and expects the abort.ls-filescheck back fails only the new directory test. Deleting the mode check fails both the100755test and the directory test.100755, a symlink, a directory, a gitlink and a missing path. It treated glob characters in a path literally, and refused a path named-h. It also ran the suite with a privateTMPDIR, once with a test forced to fail: no scratch directories were left either time. The review found no defects.shellcheckis clean onlockfix-apply.sh.