Repository navigation
Keep test git from following a hook's GIT_DIR into this repo - #133
Merged
Merged
Conversation
Git hooks, `git rebase -x` and `git bisect run` export GIT_DIR and GIT_INDEX_FILE, and in a linked worktree those are absolute paths to the real repository. Several tests run `git init`, `git add --all` and `git config` in temp dirs with only `current_dir` set, so under those variables they act on the enclosing repo instead. Committing from a worktree ran the pre-commit hook's `cargo test`, which flipped the shared .git/config to `core.bare = true` (breaking the primary clone), replaced the worktree's index with test fixtures, and, with --no-fail-fast, wrote `user.email=test@example.com` into the shared config and made fixture commits on the real branch. A regular clone isn't fully safe either: `git commit -a` and `git commit <path>` export an absolute GIT_INDEX_FILE there too. Two test-only layers: - A cargo runner (.cargo/config.toml -> dev/without-git-repo-env) clears every variable `git rev-parse --local-env-vars` lists, then execs the test binary. It runs before the test process starts, so it also covers what per-command scrubbing can't: in-process production `ls-files`/`git add` reached from unit tests, and the CLI children tests launch. It fails closed if git can't produce the list. - A tripwire, `assert_git_env_isolated()`, runs before every test git write in tests/common, the tracked_files unit tests, and runner_api's in-process stage. It refuses if any of those variables survive, covering runs that bypass the runner (a test binary run directly, `--manifest-path` from outside the repo, non-unix hosts, an overriding runner). Production code is unchanged and keeps honoring GIT_DIR/GIT_INDEX_FILE: users run `codeownership validate` from their own hooks, where `git_stage` must stage into the index being committed. The runner only affects what cargo launches inside this repo. Five regression tests in tests/git_env_isolation_test.rs cover the runner stripping the variables, failing closed without git, and preserving the binary's exit status, and the tripwire refusing both the worktree shape and a lone GIT_INDEX_FILE. Each fails when the behavior it guards is removed.
Merged
- Move the tripwire to tests/support/git_env.rs, spliced with include!() into tests/common and the tracked_files unit tests instead of duplicated. - Guard build_run_config too, since its callers stage in-process. - Re-run each tests/common helper in a child under an inherited GIT_DIR, so dropping any one helper's tripwire call fails a test. - Make the child an ignored test, so an ambient marker variable can't turn the parent test into a silent no-op. - Fix the tripwire message for runs that overrode the runner, and document the conflict with a user's own cfg(...) runner in .cargo/config.toml.
dduugg
added a commit
that referenced
this pull request
Sep 28, 2026
A minor bump. Since 0.5.0: - `allow_unowned_globs` (#135) is a new config key. It defaults to `[]`, so existing configs behave as before, but it adds a `pub` field to `Config`, which breaks downstream code that builds a `Config` with a struct literal. - Test git no longer follows a hook's GIT_DIR into this repo (#133). This only affects the test suite.
dduugg
added a commit
that referenced
this pull request
Oct 28, 2026
* Add allow_unowned_files to permit files with no owner code_ownership#140: a repo that assigns owners to only some files had no way to pass `validate` except `unowned_globs: ['**/*']`, and files matched by unowned_globs are never read, so that also switched off their `@team` annotations. With `allow_unowned_files: true`, a file in owned_globs that no mechanism assigns an owner is no longer an error, in both `validate` and `validate <files>`. Every mechanism, file annotations included, still applies to those files, so the annotations keep working. It defaults to false, and nothing else changes: unowned_globs files are still never read, which is what keeps large third-party trees cheap. Making annotations work inside unowned_globs instead would have meant opening every file those globs match. Measured on a synthetic tree of tracked vendored files, that made a cold `generate` about 7x slower at 100k files and 11x at 300k. * Take globs in allow_unowned_globs rather than a boolean A boolean could only exempt every file at once, while a glob list can also scope the exemption, e.g. `**/deprecated/**/*`, and `['**/*']` still covers code_ownership#140's case. It matches how owned_globs and unowned_globs are configured. The difference from unowned_globs is that files here are still read, so annotations and every other mechanism still apply; they just don't have to have an owner. The globs are only checked for files that have no owner, using the same glob_match used for owned_globs on every walked file. * Keep glob matching local to the validator and reuse relative_to_buf The validator imported matches_globs from project_builder, which made project_builder's copy crate-public just to share a one-line helper. It now has its own private copy, like runner.rs and file_owner_resolver.rs, and project_builder's copy is private again. validate_files' relative-path closure now calls path_utils::relative_to_buf instead of hand-rolling it. Also pin that `allow_unowned_globs: ['**/*']`, the README's way to allow every unowned file, covers files at the project root, for both `validate` and `validate <files>`. * Share one matches_globs in path_utils runner.rs, project_builder.rs and the validator each had an identical private matches_globs. They now all use path_utils::matches_globs, next to relative_to and relative_to_buf, with a unit test that `**/*` matches both root-level and nested paths. * Test allow_unowned_globs with absolute paths and the checks it keeps Nothing tested that `validate <files>` matched allow_unowned_globs against the project-relative path. Matching the raw argument instead passed the whole suite, because the only test passed a path that was already relative. A test with an absolute path now covers it. validate_files also relativizes each path once, carrying it through the owned_globs filter, instead of twice per file. Two more tests pin the README's claim that only the missing-owner check is waived: a file in an allowed glob still fails when it has more than one owner or names an invalid team. The validator now reads allow_unowned_globs from its project instead of keeping a copy, matches_globs is pub(crate) so it doesn't become public API, and its imports sit with the other crate imports. * Explain how allow_unowned_globs differs from unowned_globs The README said unowned_globs files are never read, but `for-file` reads the top of the file before ignoring its annotation. It also implied that no ownership mechanism applies to unowned_globs files, when directory, package and team-glob rules still cover them through CODEOWNERS. The real differences are that annotations count and that the other checks still run. It now also says that unowned_globs takes precedence, and how to move off `unowned_globs: ['**/*']`, the setup code_ownership#140 started from, where adding the new key alone changes nothing. The Getting Started example moves a hand-picked app file from unowned_globs to allow_unowned_globs, since unowned_globs is meant for third-party and generated code. * Bump version to 0.6.0 A minor bump. Since 0.5.0: - `allow_unowned_globs` (#135) is a new config key. It defaults to `[]`, so existing configs behave as before, but it adds a `pub` field to `Config`, which breaks downstream code that builds a `Config` with a struct literal. - Test git no longer follows a hook's GIT_DIR into this repo (#133). This only affects the test suite. * Read allow_unowned_globs with strings_or_nulls serde-saphyr rejects a list item with no value, `~` or `null`, so such an `allow_unowned_globs` entry made the whole config fail to parse. serde_yaml read a bare `-` as "" and kept `~` and `null` as text. Like the other list keys, the field now goes through strings_or_nulls, which reads all three as "", and reads an empty, `~` or `null` list as empty: serde-saphyr 2.0 does that by itself, but 2.1 stops doing it for a Vec. The empty-values config test now covers this key too. * Report allowed files that name an owner in validate <files> Given a list of files, `validate` and `gv` check each one only against CODEOWNERS, and allow_unowned_globs excused every allowed file that CODEOWNERS had no line for. That included a file whose annotation names a team that doesn't exist: without the key it is reported as unowned, and a full `validate` reports the invalid team. Now the glob excuses only a file that nothing names an owner for, counting owners as a full `validate` does, so such a file is reported as unowned again, and so is one that CODEOWNERS doesn't list yet. The owner matchers are collected once, and only when an allowed file is missing from CODEOWNERS. * Test that unowned_globs takes precedence over allow_unowned_globs The README says a file matching both is left out, but no test checked it: making allow_unowned_globs win passed the whole suite. The new test gives a file matching both an annotation naming an unknown team and checks that `gv` passes, which it can only do with the file left out. * Count a package naming an unknown team in validate <files> The previous commit made allow_unowned_globs excuse only an allowed file that nothing names an owner for, but the owner lookup missed one kind of owner. The package mappers drop a package whose package.yml or package.json names a team that doesn't exist, so a file in it still passed `validate <files>` and `gv <files>`. Without the key it is reported as unowned, and a full `validate` reports the package.yml or package.json. The lookup now adds a matcher for each such package, so the file is reported as unowned again. A full `validate` is unchanged. * Test more of how validate <files> treats allowed files Three changes to the code passed every test. The owner lookup could leave out `.codeowner` files, so an allowed file under one naming a team that doesn't exist would pass. It could be given the path as typed rather than the project-relative one, so an absolute path would miss the file's annotation. And `validate <files>` applies the precedence of unowned_globs with its own filter, where letting allow_unowned_globs win went unnoticed: an annotated file in unowned_globs is never read, but a `.codeowner` there still is. A new test catches each change. The comment on the annotation test no longer says a list of files is checked only against CODEOWNERS, which stopped being true when the lookup was added. * Say what unowned_globs still reads The README said files matching unowned_globs are left out and that validation doesn't check them. A `.codeowner`, package.yml or package.json among them is still read, though: its owner still goes into CODEOWNERS, and a team it names that doesn't exist is still reported. main behaves the same way; only the sentence is new here. * Make only absolute paths project-relative in validate <files> Reusing path_utils::relative_to_buf in validate_files changed how it reads a relative path. The helper strips the project root from any path, but the closure it replaced, like the CODEOWNERS lookup, stripped it only from an absolute one. The CLI canonicalizes the root, but a library caller can pass a relative one, as the README's example does. A relative path that starts with that root was then checked against owned_globs with the root cut off, while the CODEOWNERS lookup read it as given and found no line for it, so runner::validate and runner::generate_and_validate reported an owned file as unowned. Before that change the file was skipped. A relative path is already relative to the project root, so it is now left as given and only an absolute one has the root stripped. Such a file is skipped again, and nothing changes when the root is absolute. * Excuse only a plain project-relative path in validate <files> allow_unowned_globs excuses a file in `validate <files>` and `gv <files>` only if the owner lookup finds nothing naming an owner for it. The lookup matches only paths in the project's own form, such as app/x.rb, but it was given the path as passed. So `./app/x.rb`, `app/../app/x.rb` or an absolute path not under the canonical root, as through a symlink, found no owner, and the file was excused even when its annotation, `.codeowner` or package names a team that doesn't exist. Without the key it is reported as unowned. The excuse now drops `.` components and repeated separators from the path before matching it against allow_unowned_globs and looking up its owners, so it treats `./app/x.rb` as app/x.rb, and such a file is reported again. A path with a `..`, or an absolute one not under the project root, can't be put in that form without asking the file system, so it is never excused and is reported as without the key. The owned_globs and unowned_globs checks and the CODEOWNERS lookup read the path as before, so nothing changes without the key. * Match a package naming an unknown team by its directory The owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` gave each package naming a team that doesn't exist an unescaped glob of its directory. For a package in `packs/[typo]`, `[typo]` read as a character class, so a file in the package passed although a full `validate` reports its package.yml, while an allowed file with no owner in `packs/t` was reported as unowned. A package.yml at the project root got `/**/**`, which matches nothing, so the files under it passed too. The lookup now keeps the directory of each such package and finds an owner for any path under one. It compares paths a component at a time rather than as globs, so brackets are literal and a package at the project root covers every file. A file in such a package is reported as unowned again, as without the key, and the file in `packs/t` passes. A full `validate` is unchanged. The package test now covers package.json too. * Say the owner lookup sees only files the project reads The comments on the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` said that an allowed file is excused only if nothing names an owner for it, and that one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported. But the lookup is built from the project, which leaves out untracked files and anything under ignore_dirs. So an owner named only in such a file, as in a new file's own annotation before it is staged, or in an untracked `.codeowner` or package.yml, doesn't count, and the allowed file is excused, although without the key it is reported as unowned. A full `validate` doesn't read those files either, so it doesn't report such a file, and `validate <files>` keeps excusing it. Both comments now say that only files the project reads count, as in `validate`. Nothing else changes. * Check allow_unowned_globs in parallel in validate A full `validate` or `gv` matched each file with no owner against allow_unowned_globs in a loop on one thread. Such a file can be tried against every glob in the list, so the time grows with the number of those files times the length of the list. That is small for a few globs, but the README's Getting Started example lists a file by path, and a list of files still to be given owners can run to thousands of paths. On main such a list goes in unowned_globs, which is matched in the parallel walker. The check now runs on rayon's thread pool, like the other per-file checks in the validator. rayon's collect keeps the order of the files, so it reports the same errors in the same order. On a synthetic project of 10,000 files with no owner, each allowed by its path, the best of 15 runs of `validate` took 1.0 s before and takes 0.45 s now. * Share fixture setup between the allow_unowned_globs tests The two absolute-path tests copied the fixture, staged it and built the codeowners command line themselves, as run_on_modified_fixture does, because that helper takes its arguments before the temp root that an absolute path is built from exists. The file also named its fixture two ways, as a path for setup_fixture_repo and as a bare name for run_codeowners. The fixture is now named once, and run_on_modified_fixture is split into a builder that copies, modifies and stages the fixture and a runner for the command. validate_absolute_path joins them to validate a file by its canonical absolute path, so each absolute-path test is now a single call. The assertions are unchanged, and no test is added or removed. * Name the allow_unowned_globs tests after what they check Six tests of `validate <files>` were named as if they reported an invalid team or a package naming one, but a list of files is never checked for invalid teams. Each asserts that an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported under "Unowned files detected:". Beside test_validate_reports_invalid_teams_in_allowed_globs, which does check a full `validate` for invalid teams, the near-identical names misled. They are now named after the allowed file they report. The two `for-file` tests can't be broken by anything allow_unowned_globs changes, since `for-file` never reads the key. They are kept, now named after `for-file`, and a comment says they pin that it reports an allowed file's owner as it would without the key. Another comment says the test that a full `validate` passes relies on the fixture's CODEOWNERS listing app/deprecated/annotated_old.rb, so it also fails, with CODEOWNERS out of date, if annotations in allowed globs stop counting. Only test names and comments change. * Test more owner sources for allowed files Five changes to the code passed every test. `validate <files>` could report an allowed file with an owner even when CODEOWNERS lists it, which would fail a pre-commit hook on a commit that edits an annotated file in an allowed glob. The owner lookup could leave out team globs, or packages naming a team that exists, because the only test of an allowed file that CODEOWNERS doesn't list yet gave it an annotation. And no test ran a full `validate` with a package.yml or `.codeowner` naming an unknown team in an allowed glob, so it could stop reporting them there. New tests pass the annotated file and an unowned one to `validate` and `gv`, and run that full `validate` with each. The latter checks for each message, not just the exit code, since the `.codeowner` also gives annotated_old.rb a second owner. It also checks that the package's file, which has no owner, isn't reported as missing ownership. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a team glob and one from a package, not just an annotation. Only tests change. * Keep unit tests next to what they test When matches_globs moved from project_builder to path_utils, its test stayed behind, as did test_glob_match, which tests fast_glob itself and needed a new glob_match import in project_builder's tests. The only check of the allow_unowned_globs default went in the test of the executable_name default, so a change to it would fail a test named for another key. Both tests and their OWNED_GLOB now sit with the other matches_globs test in path_utils, calling fast_glob::glob_match by its full path as the code there does, and the import is gone. The default is checked in test_parse_config with most of the others, between executable_name and yaml_max_nodes as in the struct, and the executable_name test is back to what main has. Only tests change, and none is added or removed. * List allow_unowned_globs next to unowned_globs in the README The README's list of configuration keys put allow_unowned_globs after executable_name, with four other keys between it and unowned_globs, though the bullet is written in terms of unowned_globs. It says how the two keys differ, that unowned_globs takes precedence, and when to switch from `unowned_globs: ['**/*']` to the new key, so a reader had to look back up the list to follow it. The bullet now comes directly after unowned_globs. Its text and the rest of the README are unchanged. * Explain allow_unowned_globs for file lists, migration and old versions The README said a file in allow_unowned_globs can't have more than one owner or name an invalid team, since every other check still runs. That holds for a full `validate` or `gv`, but the README never mentioned what they do given a list of files, as from a git hook. They look each file up in CODEOWNERS and never check for more than one owner, and pass an allowed file with no line only if nothing names an owner for it. So one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Unowned files detected". The advice to replace `unowned_globs: ['**/*']` with the new key left out that CODEOWNERS then gains a line for each annotated file, so until `codeowners gv` runs, `validate` reports it out of date and `validate <files>` reports those files as unowned. It also left out that the files are then read and checked, which takes longer. The bullet now covers all of this, says to keep third-party and generated code in unowned_globs, and says the key was added in 0.6.0, since older versions ignore it without a warning and still report these files. Only the README changes. * Match a .codeowner naming an unknown team by its directory too The owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` matched a `.codeowner` only by the glob the directory mapper builds from its directory. For one at the project root that glob is `/**/**`, which matches nothing, and the mapper escapes brackets but not braces, so `app/deprecated/{legacy}/**/**` matches files in `app/deprecated/legacy` but not in `app/deprecated/{legacy}`. An allowed file under such a `.codeowner` naming a team that doesn't exist therefore passed, although a full `validate` reports the `.codeowner` and, without the key, the file is reported as unowned. The lookup now keeps the directory of each `.codeowner` naming a team that doesn't exist, as it already did for packages, and finds an owner for any path under one. A file under such a `.codeowner` is reported as unowned again, as without the key. Only the nearest `.codeowner` gives a file its owner in `validate`, but the lookup asks only whether anything names an owner. The directory mapper is unchanged, so CODEOWNERS and a full `validate` are too. A test of a package in a directory with braces is added as well. It passed before, but without it no test fails if packages are matched by globs like the directory mapper's. * Report allowed files under their own heading in validate <files> Given a list of files, `validate` and `gv` report an allowed file that CODEOWNERS has no line for only if something names an owner for it: a team that doesn't exist, or one CODEOWNERS doesn't list yet. Such a file was still listed under "Unowned files detected:", with no hint, although the config says it may go unowned. With the README's `['**/*']`, every failure for a list of files is of this kind, and in the code_ownership gem's pre-commit hook, which runs `gv` and so regenerates CODEOWNERS first, the cause is almost always a team that doesn't exist. These files now get a heading of their own. It says that they match allow_unowned_globs but name an owner that CODEOWNERS doesn't list, and suggests checking the team name or running the configured executable_name to update CODEOWNERS, as the stale-CODEOWNERS message does. Files outside the globs, and a path with a `..` or an absolute one not under the project root, are still listed under "Unowned files detected:", and both headings can appear in one run. The exit status is unchanged, and the gem only joins these lines into the error it raises. The README names the new heading, and new tests pass both kinds of file at once and check that the hint names the configured executable. * Say the config file is held to the default node limit The README said yaml_max_nodes doesn't apply to the config file itself, which can read as if the config had no limit. It is always parsed with the default of 20,000 nodes, and nothing raises that, so a config whose lists add up to about 20,000 entries fails with a `budget breached` error and, unlike a team file, no hint. A long literal allow_unowned_globs list is the likeliest way to get there, and 0.5.0, which predates #142's limits, read such a config. The sentence now gives the limit. Only the README changes. * Say that unowned_globs still reads team files The README's unowned_globs bullet says files matched there are left out of the project, then names `.codeowner`, `package.yml` and `package.json` as the files still read, which reads as the whole list. A team file is another: the project builder matches team_file_glob before owned_globs and unowned_globs, so a team file in unowned_globs still defines its team and keeps its own line in CODEOWNERS. The bullet now says so in a sentence of its own, since the invalid-team check the sentence before it mentions doesn't apply to team files. Only the README changes. * Match a .codeowner naming an unknown team only by its directory The owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` matches the directory of a `.codeowner` naming a team that doesn't exist as a path prefix, but it still took the glob the directory mapper builds from that directory as well. The mapper escapes brackets but not braces, so the glob it builds for one in `app/deprecated/{legacy}` matches files in `app/deprecated/legacy` instead. An allowed file there, which nothing names an owner for, was therefore listed under the heading for files that name an owner CODEOWNERS doesn't list, and the run failed instead of passing. The lookup now leaves out the glob for such a `.codeowner`, as the prefix covers just the files under its directory, so that file passes and a file under the `.codeowner` is still reported. The glob for a `.codeowner` naming a team that exists is kept, since it names an owner for files that CODEOWNERS may not list yet. The directory mapper is unchanged, so CODEOWNERS and a full `validate` are too. The braced test now also passes a file in `app/deprecated/legacy` and fails without the change. The test of an allowed file that CODEOWNERS doesn't list yet covers an owner from a `.codeowner` too, since without it no test fails if the lookup drops every `.codeowner` glob. * Look up a non-plain allowed path in CODEOWNERS by its plain form Given a list of files, `validate` and `gv` look each one up in CODEOWNERS by the path as given, but match a file with no line against allow_unowned_globs and the owner lookup by its plain project-relative path. So an allowed file that CODEOWNERS lists, such as an annotated one, missed its line when passed as `./app/x.rb`, `app//x.rb` or `app/./x.rb`, the owner lookup found its owner, and the file was listed under the heading for files that name an owner CODEOWNERS doesn't list. That is false, and neither checking the team name nor regenerating CODEOWNERS made it pass. The `./` form passed before allowed files were matched by their plain path. Such a file is now looked up in CODEOWNERS again by its plain path, and passes if that finds a line. The second lookup is made only when the plain path differs from the one given and the owner lookup has found an owner, so only a file that would otherwise be listed pays for it. A file outside allow_unowned_globs is still looked up only by the path as given, as on main, so nothing changes without the key. A new test passes the annotated file to `validate` and `gv` by each of those paths and fails without the change. The test of a `./` path now allows only `app/deprecated/**/*`, which matches the plain path but not the one given, so it also catches allow_unowned_globs being matched against the path as given, which no test did before. * Don't suggest updating CODEOWNERS that gv <files> has just written Given a list of files, `gv` writes CODEOWNERS before it looks each one up, so an allowed file it lists under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list" is almost always there because its annotation, `.codeowner` or package names a team that doesn't exist. The heading still suggested running the configured executable_name to update CODEOWNERS, which can't help. The default, `codeowners generate`, writes the same file again, and the code_ownership gem names `bin/codeownership validate`, which by default runs `gv` again. For a file under an empty `.codeowner`, "check the team name" didn't say where to look either. Both commands now say to check the team name in each file's annotation, `.codeowner` or package, and only `validate` still suggests running the executable, as its CODEOWNERS may be out of date. The heading's first sentence, which the README quotes, and the exit status are unchanged. The tests now expect the heading each command prints, and a new one runs `gv` with the gem's executable_name and checks that it doesn't suggest updating CODEOWNERS. They fail without the change. * Test a package.json owner and a sibling directory for allowed files Two changes to the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` passed every test. It could leave out package.json files naming a team that exists, because each one the tests add names a team that doesn't, and those are matched by their directory instead. `validate` would then pass an allowed file in such a package that CODEOWNERS doesn't list yet, rather than list it under the heading for files that name an owner CODEOWNERS doesn't list. It could also match the directory of a package or `.codeowner` naming a team that doesn't exist as a string prefix rather than a component at a time, so a package in `packs/typo` would name an owner for an allowed file with none in `packs/typo2`, which would be listed under that heading instead of passing. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a package.json, as it did one from a package.yml and a `.codeowner`, and a new test passes the file in `packs/typo2` to `validate` and `gv`. Each fails with its change. Only tests change. * Share config writing between the allow_unowned_globs tests Thirteen fixture changes in the allow_unowned_globs tests replaced config/code_ownership.yml with an inline YAML string, each quote and newline escaped, and several were copies of one another: owning `**/*.rb` and allowing `**/*` three times, and owning `{app,config,packs}/**/*.rb` and allowing `packs/**/*` three more. Which globs a test's config owned or allowed had to be picked out of the escapes. A Config struct now takes each key's globs by name, and its write method renders the YAML, leaving out a key with no globs so that unowned_globs keeps its default. Every fixture change that writes the config now uses it, including name_an_executable, which appended its key to the fixture's config. The two that copied allow_every_file now call it. Every fixture a test builds has the same files, byte for byte, as before, and no test is added, removed or changed in what it checks. Only tests change. * Correct comments on the owner lookup and the config's node limit Two comments on the owner lookup behind allow_unowned_globs in `validate <files>` and `gv <files>` said an annotated file, `.codeowner` or package file names no owner there if it is under ignore_dirs, but the project builder prunes only a directory at the project root that ignore_dirs names. With the default ignore_dirs, an annotation naming a team that doesn't exist keeps a file in `app/tmp` from passing, though not one in `tmp`. They now say a top-level directory named in ignore_dirs. The one in validate_files also said a file whose annotation, `.codeowner` or package names a team that doesn't exist, "or that CODEOWNERS doesn't list yet", is still reported, which can be read as a team that CODEOWNERS doesn't list. It now says "or whose owner CODEOWNERS doesn't list yet", as the README does, since an owner that CODEOWNERS lists for other files counts too. The comment where the config is read said that at a few hundred nodes it never needs a higher limit, but a long literal allow_unowned_globs list can run to thousands of entries, and the README says the config is held to the default of 20,000 nodes, about one per list entry. It now says the config's lists can hold about 20,000 entries in all. Only comments change. * Clarify the README on team files and what the owner lookup reads In the README's unowned_globs bullet, "So is a team file, which still defines its team and keeps its own line in CODEOWNERS" followed "an invalid team in it is still reported", so the elliptical "So is" could be read as saying that a team file is still reported. It now says that a team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. The allow_unowned_globs bullet said that, given a list of files, an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported, but the owner lookup that decides this sees only the files the project reads. An untracked file's own annotation names no owner there, nor does an untracked `.codeowner` or package file, or any of them in a top-level directory named in ignore_dirs, as in a full `validate`. So `validate app/new.rb` passes a new allowed file whose annotation names a team that doesn't exist, and reports it only once the file is staged. A sentence after the one quoting the heading now says so. It says that such a file doesn't count, rather than that only a tracked one does, since outside a git repository no file is left out for being untracked. Only the README changes. * Give executable_name's real default in the README The README said executable_name defaults to `'codeowners'`, but the default, here and on main, is `codeowners generate`. Its example config set `bin/codeownership` for the Ruby gem, while the gem passes `bin/codeownership validate`, which regenerates CODEOWNERS by default. Both the stale-CODEOWNERS message and the new hint for allowed files in `validate <files>` print this value as the command to run to update CODEOWNERS. The gem's own runs aren't affected, since the value it passes replaces the config's, but a config copied from the example made `codeowners validate` tell users to run `bin/codeownership`, which only prints the gem's usage. The bullet now gives the real default and says the key is the whole command these messages tell you to run, rather than a command name, and the example sets `bin/codeownership validate`. Only the README changes. * Rename allow_unowned_globs to optionally_owned_globs `allow_unowned_globs` reads as `allow_` followed by the name of the `unowned_globs` key, as though it switched on or loosened that key. The relationship is the reverse: the new key narrows `owned_globs`, whose files it still reads and checks, and has nothing to do with `unowned_globs`, which takes precedence over it. The `allow_` prefix also reads like a boolean, as rubyatscale/code_ownership#140 proposed with `allow_unowned_files: true`, while the other list keys are noun phrases. `optionally_owned_globs` sits between the two existing keys: files in `owned_globs` must have an owner, files that also match `optionally_owned_globs` may have one, and files in `unowned_globs` are left out. Since the name could be read as adding files beyond `owned_globs`, the README now says it doesn't, and that a file must match both. The key, the `Config` and `Project` fields, the heading that `validate <files>` prints, the README, the fixture directory, the test file and the test names all use the new name, and prose that called these files "allowed" now calls them optionally owned. 0.6.0 hasn't been released, so nothing reads the old name and no alias is kept. * Stop at the first owner in OwnerLookup::has_owner has_owner only needs to know whether anything names an owner, but it called FileOwnerFinder::find, which runs every owner matcher, clones each matching source into a map of teams and collects the owners, all to check that the list isn't empty. With `optionally_owned_globs: ['**/*']`, every file `validate <files>` is given that has no line in CODEOWNERS reaches it. find returns an owner exactly when some matcher names a team: a matched directory always survives as the most specific one. So has_owner now asks whether any matcher names a team, and stops at the first. Fresh Eyes reported this as an info-level performance finding.
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.
Git hooks export
GIT_DIRandGIT_INDEX_FILE, and in a linked worktree they're absolute paths to the real repository. Several tests rungit init,git add --all, andgit configin temp dirs with onlycurrent_dirset, so under those variables they act on the enclosing repo instead. The repo's own pre-commit hook runscargo test, so committing from a worktree corrupts the repo.What happens today
Measured with a probe hook (git 2.55.0):
GIT_INDEX_FILE.git/index(relative, resolves inside each temp repo)GIT_DIROne commit from a worktree on
main:.git/configtocore.bare = true, which broke the primary clone (git status: "this operation must be run in a work tree");--no-fail-fast, also wroteuser.email=test@example.cominto the shared config and made fixture commits on the real branch.The hook fails partway, so nothing gets committed, but the damage is done. A regular clone isn't fully safe either:
git commit -aandgit commit <path>export an absoluteGIT_INDEX_FILEthere as well. The same exposure applies togit rebase -x 'cargo test'andgit bisect run cargo testin a worktree.Fix — two test-only layers
A cargo runner.
.cargo/config.tomlroutes every binary cargo launches throughdev/without-git-repo-env:git rev-parse --local-env-varsis git's own list of repository-local variables, the same set it clears when running in a different repo. The runner fails closed: if git can't produce the list, the tests don't run. The simplerunset $(git rev-parse --local-env-vars)fails open, because a failed git leavesunsetwith no arguments. Clearing the environment before the test process starts also covers what per-command scrubbing can't reach: in-process productionls-filesandgit addcalled from unit tests, and every CLI childassert_cmdlaunches.A tripwire.
assert_git_env_isolated()runs inside everytests/commonhelper that touches a repo (init_git_repo,git_add_all_files,git_reset_all,is_file_staged, andbuild_run_config, whose callers stage in-process), in thetracked_filesunit tests, and beforerunner_api's in-process stage. It refuses to proceed if any of those variables survive. That covers runs that bypass the runner: a test binary run directly,--manifest-pathfrom outside the repo, non-unix hosts, or an overriding target runner. It guards the shared helpers, not each git call: tests that stage or rungenerateare covered because they build their repo through those helpers first, so a new test that skips them should call it too. It lives intests/support/git_env.rsand is spliced withinclude!()into bothtests/commonand thetracked_filesunit tests, because unit tests can't importtests/commonand integration tests link the library built withoutcfg(test). That shares one copy without adding test support to the crate's public API.Why not change production code or the hook
Production behavior is unchanged, deliberately. Users run
codeownership validatefrom their own pre-commit hooks, wheregit_stage()must stage CODEOWNERS into the index being committed, andls-filesmust see that index. The runner only affects what cargo launches inside this repo; the library and the released binary never see it..rusty-hook.tomlis untouched. rusty-hook runs its command withsh -cand inherits the environment, so the existing hook'scargo testalready goes through the runner. Fixing only the hook would have left manual runs,rebase -x,bisect run, and other hook managers exposed, and any shell syntax in the hook string breaks Windows'scmd /C.Verification
main, a worktree commit reproduces the corruption above;GIT_DIRandGIT_INDEX_FILEexported by hand at a victim repo leave it byte-identical with this change. Onmain, the same run corrupts it.tests/git_env_isolation_test.rs. They check that the runner strips the variables, fails closed without git, and preserves the test binary's exit status. They also re-run eachtests/commonhelper in a child process under the worktree shape, andinit_git_repounder a loneGIT_INDEX_FILE, and check that it refuses. The child is an#[ignore]d test, so a stray marker variable can't turn the parent into a no-op. Each fails when the behavior it guards is removed; for example, a runner that swallows exit status fails withleft: Some(0), right: Some(3).cargo fmt --checkandcargo clippy --all-targets --all-features -- -D warningsare clean;cargo testpasses 168/0 across 27 binaries, and five runs at--test-threads=16pass 840/0. Removing the tripwire call from any single helper failstest_git_helpers_refuse_inherited_repo_env. Confirmed under Linux dash and macOS/bin/sh.Worth knowing
cargo runinside this repo also goes through the runner, so it doesn't see these variables either. One consequence:cargo run -- generate --stagefrom a hook duringgit commit -aorgit commit <path>stages into.git/indexrather than the hook's temporary index. The released binary is unaffected.[target.'cfg(...)'] runner, cargo refuses to choose between the two (several matching instances of target.'cfg(..)'.runner). Override withCARGO_TARGET_<TRIPLE>_RUNNER; the tripwire still guards git..cargo/config.tomlsays the same.cfg(unix). Elsewhere, the tripwire still refuses before any git write made through the shared test helpers.--local-env-varslist, so when the runner is bypassed, harmless variables likeGIT_CONFIG_PARAMETERS(set by agit -cparent) also make it refuse. That's deliberate: it fails closed.rebase -x-ing across commits older than this one, from a worktree, is still exposed, since those commits don't have the runner.git rev-parseoutside both layers (cargo runs build scripts directly). It's read-only and fails harmlessly under an inheritedGIT_DIR.Sent with Claude Code