search

hmarr / codeowners

🔒 Command line tool and Go library for CODEOWNERS files
MIT License
167 stars 19 forks source link

Doesn't correctly follow rules for ownership in nested directories #2

Closed ThisIsMissEm closed 3 years ago

ThisIsMissEm commented 3 years ago

I've just noticed that this project parses the codeowners file incorrectly according to Github's specification (examples), given the following lines:

* user-a
src/* user-b

Then the file at src/foo/bar.js should actually be owned by user-a, this is because of this logic:

# In this example, @doctocat owns any files in the build/logs
# directory at the root of the repository and any of its
# subdirectories.
/build/logs/ @doctocat

# The `docs/*` pattern will match files like
# `docs/getting-started.md` but not further nested files like
# `docs/build-app/troubleshooting.md`.
docs/*  docs@example.com

(from https://docs.github.com/en/free-pro-team@latest/github/creating-cloning-and-archiving-repositories/about-code-owners )

Essentially the /* only matches direct descendants, not all descendants of a directory. This logic is the same for Gitlab too.

ThisIsMissEm commented 3 years ago

I suspect GitHub is actually using fnmatch(3) rules, with the FNM_PATHNAME | FNM_LEADING_DIR flags, there is a go library that seems to implement the logic of fnmatch(3): https://github.com/danwakefield/fnmatch/

hmarr commented 3 years ago

Thanks for pointing this out – you're totally right. And sorry for missing this issue until now! It's actually using pathspec, so I've updated the pattern matching logic to follow almost exactly what that library does. I tried it out with the example you provided and it seems to fix that behaviour.

I also pulled the tests cases out into JSON file and ran them through the internal codeowners library to verify the new logic matches the canonical one, at least for the cases covered in that file.

I'm planning on running a large project that uses code owners through both this library and the canonical GitHub one and comparing the results, which should help catch any edge cases I've missed. Once I've done that I'll tag a new release.

hmarr commented 3 years ago

I confirmed the new matching implementation is consistent with the canonical GitHub one over a large project, so I just released it in v0.3.0. Thanks again for reporting this!