zendesk / setup-check-codeowners

A script to check .github/CODEOWNERS, and a GitHub Action to install it
MIT License
1 stars 2 forks source link

/* matches all files in a directory but not nested directories #25

Closed grosser closed 2 years ago

grosser commented 2 years ago

failing test to reproduce issue found in https://github.com/zendesk/kennel/pull/12287

Discussion of the problem:

check-codeowners aims, of course, to match Github's codeowners behaviour (closed source, one assumes), and the best docs we have for that (as far as I know) is this: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners . However, that doc contains problems:

Specifically (and this was gleaned from lots of experimentation), patterns which end in /* (including literally just /* itself) behave differently in gitignore vs in codeowners:

Therefore that's the bug we have to address.

When check-codeowners was created, part of the philosophy was to use git itself to process the patterns, as "gitignore" patterns - because the codeowners docs claim to be (almost) the same as gitignore. Hence why this bug came to exist. The bug fix here is to have an exception for "if the pattern ends in /*, then remove any subdirectory matches.

zdrve commented 2 years ago

Not sure of the fix yet, but here's what's going on:

In order to do the brute-force matching, i.e. work out exactly which files are matched by each individual pattern, it uses git ls-files -z --cached --ignored --exclude PATTERN - in other words: if you ignore PATTERN and then show all the ignored files, what files do you get? That should be what the pattern matches, right?

Wrong.

In the test in this PR,

PATTERN=/*

files are
/.github/CODEOWNERS
/file1
/foo/file1

So the git ls-files ignores /.github and /file1 and /foo and if you then list the ignored files, that means... all of them. Because files inside an ignored directory are themselves ignored.

In other words, "if you ignore PATTERN and then show all the ignored files, what files do you get?" is not the same as "which files match PATTERN?".

Not sure of the fix yet.

grosser commented 2 years ago

FYI I used https://github.com/timoschinkel/codeowners-cli to verify locally, so maybe we can re-evaluate the tools out there and contribute our needs instead of re-inventing here ...

zdrve commented 2 years ago

Certainly worth checking. Cost of deciding to keep: that we need to accept / fix any bugs, and bear the continued costs of maintenance, and the cost of not gaining any extra features that the alternative might have. Cost of deciding to ditch: time invested into switching, plus the costs of any lost features, plus reliance on third party (with its security implications, version bumping, and so on).

grosser commented 2 years ago

https://github.com/zendesk/kennel/pull/12288 codeowners seems to work fine ... if we build some go tool around it we might be able to make it fast too ...

zdrve commented 2 years ago

For the record I've changed my mind about the analysis in https://github.com/zendesk/setup-check-codeowners/pull/25#issuecomment-1114028125 . The deeper problem is that https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#syntax-exceptions is self-contradictory, and the reality of how codeowners works doesn't seem to match that doc.