|
| 1 | ++++ |
| 2 | +title = "Review Guidelines" |
| 3 | +template = "page.html" |
| 4 | ++++ |
| 5 | + |
| 6 | +This page describes what we expect from pull requests across the uutils |
| 7 | +projects ([coreutils](https://github.com/uutils/coreutils), |
| 8 | +[findutils](https://github.com/uutils/findutils) and the others) and how reviews |
| 9 | +are carried out. It is meant for both **contributors** who want to know what a |
| 10 | +reviewer looks for, and **reviewers** who want a shared checklist. Each |
| 11 | +project's `CONTRIBUTING.md` links here; the rules below apply to all of them. |
| 12 | + |
| 13 | +## The one rule that cannot be bent |
| 14 | + |
| 15 | +> uutils is **original** code. We **cannot** accept any change based on the GNU |
| 16 | +> source code, and you **must not even link to** the GNU source in an issue or |
| 17 | +> PR. A reviewer will reject a contribution that appears to be derived from GNU |
| 18 | +> (or any other strongly-licensed implementation such as GPL/LGPL code). |
| 19 | +
|
| 20 | +It is fine to look at permissively-licensed implementations |
| 21 | +([Apple's file_cmds](https://github.com/apple-oss-distributions/file_cmds/), |
| 22 | +[OpenBSD](https://github.com/openbsd/src/tree/master/bin)) and to read the GNU |
| 23 | +*manuals* and man pages — just never the GNU *source*. |
| 24 | + |
| 25 | +## What a reviewer expects before merging |
| 26 | + |
| 27 | +A pull request is ready to be merged when it meets all of the following. If you |
| 28 | +check these before requesting a review, your PR will move much faster. |
| 29 | + |
| 30 | +- **It passes CI.** The test suite is green (allowing for intermittently |
| 31 | + failing tests), `rustfmt` is satisfied, and there are no `clippy` warnings. |
| 32 | +- **It compiles without warnings on every CI platform.** Use `#[cfg(...)]` for |
| 33 | + platform-specific code rather than breaking other targets. |
| 34 | +- **It is small and self-contained.** A series of small PRs gets merged far |
| 35 | + faster than one large one. Unrelated changes belong in separate PRs. |
| 36 | +- **It has a descriptive title.** Describe the problem solved, e.g. |
| 37 | + `ls: fix version sort order`, not `Fix #1234`. Prefix with the utility name |
| 38 | + when relevant. |
| 39 | +- **New behavior comes with tests.** Our test suite is fast; regressions should |
| 40 | + be caught by a test. Code coverage should not regress. |
| 41 | +- **If possible, it was discussed first.** For anything non-trivial, open (or comment on) an |
| 42 | + issue *before* writing the code, so effort isn't wasted on a change we can't |
| 43 | + merge. |
| 44 | +- **It follows GNU behavior** for options and output, verified against the GNU |
| 45 | + manual or output — never the GNU source. |
| 46 | + |
| 47 | +### Commit hygiene reviewers care about |
| 48 | + |
| 49 | +- Small, atomic commits with a clean history. |
| 50 | +- Informative messages annotated with the component, e.g. |
| 51 | + `cp: do not overwrite with -i` or `uucore: add support for FreeBSD`. |
| 52 | +- Don't move code around unnecessarily - it makes diffs hard to review. If a |
| 53 | + move is needed, do it in its own commit. |
| 54 | + |
| 55 | +### Coding expectations reviewers check against |
| 56 | + |
| 57 | +- **No `panic!`** - avoid `.unwrap()`, `panic!` and stray `println!`. A |
| 58 | + justified `unreachable!` needs a comment. |
| 59 | +- **No `exit`** - utilities must be embeddable, so avoid `std::process::exit` |
| 60 | + and friends. |
| 61 | +- **Minimal `unsafe`** - generally only for FFI, always with a `// SAFETY:` |
| 62 | + comment. Performance is rarely a good enough reason. |
| 63 | +- **`OsStr`/`Path` over `str`/`String`** for paths, since paths may not be valid |
| 64 | + UTF-8. |
| 65 | +- **Macros sparingly**, and **comments that explain *why***, kept up to date. |
| 66 | + |
| 67 | +## For contributors: getting your PR reviewed |
| 68 | + |
| 69 | +- You don't need to ping a maintainer the moment you open a PR. |
| 70 | +- If you get no response within a few days, it's fine to request a review. |
| 71 | +- If after a week there's still no review, ping the maintainers on |
| 72 | + [Discord](https://discord.gg/wQVJbvJ) (`#coreutils-chat` for coreutils). |
| 73 | +- You know your code best - please resolve merge conflicts on your branch |
| 74 | + yourself (`git merge main` or `git rebase main`, your choice). Ask for help if |
| 75 | + you get stuck. |
| 76 | +- When you address review feedback, fold the fixes into the relevant commits |
| 77 | + (`git commit --fixup` / `git absorb`) to keep history clean. |
| 78 | + |
| 79 | +## For reviewers: how we review |
| 80 | + |
| 81 | +- **Double-check a human's work.** A reviewer is there to verify a contributor's |
| 82 | + reasoning, not to launder unreviewed machine output. Expect the author to be |
| 83 | + able to explain and justify every line. |
| 84 | +- **Watch for GNU/GPL provenance.** Be especially careful with AI-assisted |
| 85 | + patches: assistants are trained on GPL sources and can reproduce them |
| 86 | + verbatim, which we cannot accept. |
| 87 | +- **Keep comments short and actionable.** Prefer simple, one-line comments on |
| 88 | + the specific line, so the author knows exactly what to change. |
| 89 | +- **Push back on** long-winded code, duplication, needless complexity, and |
| 90 | + changes that arrive without tests. |
| 91 | +- **Confirm the basics** from the checklist above (CI, scope, title, tests, |
| 92 | + style) rather than re-deriving them each time. |
| 93 | + |
| 94 | +## AI-assisted contributions |
| 95 | + |
| 96 | +AI-assisted contributions are allowed, but the same standards apply as for any |
| 97 | +other patch. If you use an AI tool, **you** are responsible for the result: you |
| 98 | +should understand every line, be able to justify it in review, and make sure the |
| 99 | +output is not derived from GNU or other GPL code. Keep patches small and |
| 100 | +self-review the diff carefully before opening the PR. |
| 101 | + |
| 102 | +## See also |
| 103 | + |
| 104 | +- The full `CONTRIBUTING.md` in each repository |
| 105 | + ([coreutils](https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md), |
| 106 | + [findutils](https://github.com/uutils/findutils/blob/main/CONTRIBUTING.md)) |
| 107 | +- `DEVELOPMENT.md` for setting up your environment |
| 108 | +- Our [Code of Conduct](https://github.com/uutils/coreutils/blob/main/CODE_OF_CONDUCT.md) |
0 commit comments