Require a comment for each patch?

Hi Nixpkgs maintainers,

I figured the following would be a de facto standard, because it helps other maintain a patch:

  • patches have a comment describing either the upstream URL or a reason why the patch wasn’t upstreamed

… but maybe it’s not a standard yet?
What would be a valid reason not to have such a comment?

See doc/reviewing-contributions: Add points about patches by roberth · Pull Request #240136 · NixOS/nixpkgs · GitHub


Sounds OK. I suppose you don’t mean trivial cases like:

  • patch is fetched from an upstream URL (so doesn’t need this comment)
  • it’s trivial and obviously Nix(OS)-specific (e.g. adjusting some paths)

Hmm. On the first pass I read the below as meaning a comment on the intent/effect of the patch fetched from the upstream URL (which may not be evident from the URL or even the patch itself):

Well, either way the point needs reformulation if we can’t even agree on its meaning.

1 Like

Ideally, we each patch would include both a short explanation (so that people looking at the expression can know why it is needed at a glance), and a upstream status/URL (when more in depth analysis is being done).

This is the explanation nixpkgs-hammering links to when it encounters a patch without a comment:

In most cases an explanatory comment is good. I think it is also important to pick a good patch name which I feel like which is often neglected when using fetchpatch. That does not only improve code readability, but also causes the build output to make more sense (maybe we should remove the default for name in fetchpatch…). In some cases, the patch name alone is enough for the entry in patches to make sense.