CONTRIBUTING: allow fetching unaccepted changes if the URL is stable #401863
Conversation
|
cc @alyssais The common case (GitHub) does GC such commits, so this will probably nudge people in the wrong direction on average. |
Hmm, @eclairevoyant claimed otherwise in #400873 (comment). The only official statement from GH I found is this blog post where they state:
So I think that GH commits could be considered safe. |
|
This does not match our experience in practice. |
|
Arbitrary example: can you look at previous force pushes of #123456? I can't. |
|
“Nothing up my sleeve” number: #54321. PR from 2019, the first commit in the force‐push event is a 404. Also true of #123456 from 2021, and #234581 from 2023 (had to increment until I found a PR with a force push). GitHub do routinely run garbage collection; IIRC they state somewhere that it happens every 50 pushes. If they started explicitly retaining force‐pushed PR commits at some point, it would have to have been within the past couple of years, and I haven’t seen anything suggesting it’s changed. |
4e3cb3a to
0c69a46
Compare
|
I see. I still think the current wording is confusing since stable links to unmerged changes are possible. I've changed the wording around garbage collection to be stronger and namedropped GtiHub as an example. |
Reword documentation to allow using `fetchpatch` for changes that are not yet merged upstream. Additionally, put a greater emphasis on URL stability. fixes NixOS#400873
0c69a46 to
519de62
Compare
There was a problem hiding this comment.
Much better. If you want, you could include https://lore.kernel.org/ as an example of patches that do have likely-stable URLs.
|
Thanks, this looks good. I appreciate the mention that GitHub and other forges shouldn’t be relied upon to act this way. I do think that “unmerged patches should be vendored” is generally the right heuristic, because the kinds of settings where you do have stable archiving of patches (mailing lists, mostly?) are also the kind where it’s often hard to get a usable direct URL to the patches. The kernel is certainly an exception, though. |
I agree that it's a good heuristic but if we want to keep it should be framed as an advice and not as a rule. |
There was a problem hiding this comment.
Hmm, @eclairevoyant claimed otherwise in #400873 (comment).
I was incorrect, I checked some recent PRs and their commits are gone.
In any case the current PR makes the guidelines clearer and more consistent.
| ### Vendoring patches | ||
|
|
||
| In the following cases, a `.patch` file _should_ be added to Nixpkgs repository, instead of retrieved: | ||
|
|
||
| - solves problems unique to packaging in Nixpkgs | ||
| - is already proposed upstream but not merged yet |
There was a problem hiding this comment.
- - is already proposed upstream but not merged yetisn't that the case when we should vendor?
There was a problem hiding this comment.
it can be misleading. people read it to mean "if it's not even proposed upstream don't vendor" even though that's not what it says of course
Reword documentation to allow using
fetchpatchfor changes that are not yet merged upstream. Additionally, put a greater emphasis on URL stability.fixes #400873
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.