Maintainers: please don't push directly to master/release branches

[domenkozar]

It seems like the community is close to a consensus for strongly discouraging direct pushes to master and release branches.

Mainly because nixpkgs is a huge project and we've hit the critical mass where things break too often and changes benefit from a review. We can not rely on discipline any longer.

There are exceptions that are allowed for the time being:

• automatic updates: those should transition to https://cli.github.com/manual/gh_pr_create or plain github actions
• release managers
• time-sensitive changes (evaluation fix, security fix, etc)
• trivial changes (typos)

For trivial changes like typos, you can prepend [skip ci] to commit message.

Note that there was NixOS/rfcs#79 that will get a successor to formalize this.

[7c6f434c]

Mainly because nixpkgs is a huge project and we've hit the critical mass where things break too often. We can not rely on discipline any longer.

Break = channel stall? I wonder how much average channel advance frequency is actually affected by broken direct pushes as opposed to everything else.

There are exceptions that are allowed for the time being:

Is leaving out backports a typo?

Note that there was NixOS/rfcs#79 that will get a successor to formalize this.

Not sure how relevant one more failed RFC is to anything…

[domenkozar]

Break = channel stall? I wonder how much average channel advance frequency is actually affected by broken direct pushes as opposed to everything else.

Anything that doesn't pass the CI (excluding builds for the moment), which breaks all subsequent commits and PRs. This has happened many times in the past and PRs give a chance for everyone to review things.

Is leaving out backports a typo?

I don't understand.

[FRidh]

Looking back at the old RFC, my comments regarding staging-(next) were irrelevant; as long as pushes remain possible to staging(-next), it is fine. Now, I would suggest to block pushing to master, and discourage on all other branches. That way, backports will at least keep happening. But let's discuss that further in an RFC.

[andir]

Looking back at the old RFC, my comments regarding staging-(next) were irrelevant; as long as pushes remain possible to staging(-next), it is fine. Now, I would suggest to block pushing to master, and discourage on all other branches. That way, backports will at least keep happening. But let's discuss that further in an RFC.

I still wonder where the idea of "just pushing a backport" is different from a change to master comes from. It might as well be a breaking change even if the commit applies. IMHO maintainers should also be involved in those and especially in stable updates. Those might each have a different impact from package to package.

[FRidh]

From a "does it break something" point of view, I agree, it is not different. Impact wise, however, it is different; there are far fewer PR's made against stable branches so the impact of a failing CI on other PR's is less big. On the other hand, it could block a stable channel from advancing. I guess its a matter of what odds do you accept.

[7c6f434c]

Anything that doesn't pass the CI (excluding builds for the moment), which breaks all subsequent commits and PRs.

In this sense, it is a trade-off in terms of development annoyance, not a user-facing breakage. For all the twelve years I remember, there were some annoying breakages on master from time to time (actually now they matter less than initially when Hydra substituters were much more entire-channel-or-nothing)

> Is leaving out backports a typo? I don't understand.

You start with saying «master and release branches». Recently I have been under impression that a non-release-manager committer doing a backport to a release branch via a direct push is considered acceptable, because backports are not done enough. (I don't have an understanding what is backport-worthy in the first place as I never use stable branches)

[domenkozar]

this sense, it is a trade-off in terms of development annoyance, not a user-facing breakage.

Yes, we're talking about users that contribute to Nixpkgs.

You start with saying «master and release branches». Recently I have been under impression that a non-release-manager committer doing a backport to a release branch via a direct push is considered acceptable, because backports are not done enough. (I don't have an understanding what is backport-worthy in the first place as I never use stable branches)

It is acceptable but strongly discouraged.

Note that we have no proof that going through PRs will mean there are fewer backports, even more so if we create automation (a shell script with a few lines calling github cli).

Note 2: Removing the power from people always triggers "oh I don't like that so I won't participate" response, but that's not a sufficient proof :)

Note 3: We could use something like https://github.com/tibdex/backport to make this even easier than it ever was.

[7c6f434c]

> You start with saying «master and release branches». Recently I have been under impression that a non-release-manager committer doing a backport to a release branch via a direct push is considered acceptable, because backports are not done enough. (I don't have an understanding what is backport-worthy in the first place as I never use stable branches) It is acceptable but strongly discouraged. Note that we have no proof that going through PRs will mean there are fewer backports, even more so if we create automation (a shell script with a few lines calling github cli). Note 2: Removing the power from people always triggers "oh I don't like that so I won't participate" response, but that's not a sufficient proof :)

It's true I can't say that about myself — I do not do backports at all anyway.

Note 3: We could use something like https://github.com/tibdex/backport to make this even easier than it ever was.

I sure support the part about enabling that…

[vcunat]

I've been still doing direct pushes to nixpkgs master occasionally, always in cases that:

• are unlikely to get any feedback (from previous experience), and
• don't change anything critical, and
• are very unlikely to break anything (or anything else, at least, e.g. trivial version+hash bumps)

I could do them through PRs, though it'd be some extra hassle that didn't seem worth it under those conditions. (They don't seem covered by the "trivial changes" bullet, as you formulate it.) Anyway, not a big deal to me if I have to PR them.

[bergkvist]

I'm not a member of the Nix organization, but some potentially useful questions to ask here are:

• What is the probability that a push directly to master by a NixOS-member will introduce a new issue?

• How much lower is the probability that a pull request by a NixOS-member will introduce a new issue compared to a direct push?

• Does forcing everything through PRs motivate less atomic, and larger commits/changes?

• Enforcing PRs will likely reduce the output of the core NixOS members. By how much?

A practical example

(with made up numbers)
Pushing directly to master: 10% chance of introducing breakage
Creating a PR: 3% chance of introducing breakage
Having to go through PRs instead of pushing directly halves the number of bugfixes that are shipped

With these made up numbers, what will lead to the fewest issues over time? Allowing direct pushes to master, or forcing everything to go through PRs?

---

It is not obvious that enforcing PRs for core maintainers will lead to a more "polished product". Rather, over time - it might not be able to keep up, and end up accumulating more issues.

Measuring/estimating what these numbers actually are might provide some useful insights.

[grahamc]

In my opinion a major part of the issue here is more than just reducing chances of introducing problems, but adding a chance for audit and review. In this day and age the number of projects where just pushing to the main branch is acceptable is quite low. I feel it is our responsibility as a distribution to take our role quite seriously and reduce the ways weird patches can be submitted without audit.

[bergkvist]

@grahamc Sure, auditing/reviewing everything sounds like a nice ideal, but this might be an issue:

As has been already pointed out, the four eyes principle is hugely impractical for Nixpkgs, when we can't even get four eyes on mostsmall PRs as it is.

NixOS/rfcs#79 (comment)

[grahamc]

Yes I understand, however there is a big difference between no opportunity to review and a self-merged PR which was unreviewed.This is a step in the right direction.

[vcunat]

The current accumulated count of open vs. closed PRs doesn't seem that bad (2–3 %).

[grahamc]

The current accumulated count of open vs. closed PRs doesn't seem that bad (2–3 %).

This makes no requirement on review, either. It simply gives the opportunity for review.

[vcunat]

If so, we might want to define how long an opportunity should be required. We have at least one PR in nixpkgs that was self-merged within a couple seconds ;-) (you might recall that from a release manager presentation on some NixCon)

[bergkvist]

Isn't it also possible to comment on individual commits? What is the difference between commenting on an already merged PR and a commit? Two things I can think of:

• Might be harder to find/search for comments on commits in the GitHub UI
• Test suite doesn't run

[grahamc]

If so, we might want to define how long an opportunity should be required. We have at least one PR in nixpkgs that was self-merged within a couple seconds ;-) (you might recall that from a release manager presentation on some NixCon)

Right. I'm actually not too worried about this, though. Simply having people use PRs and having ofborg check the evaluation is a net win. Reducing the direct push is a significant win. Removing the ability for almost everybody to directly push is another significant win. IMO creating more policy later is okay.

[vcunat]

@bergkvist: you can comment on any commits, but I expect the intention is to get the review (and the implied fixes) before merging.

[bergkvist]

Bit rot is a potential downside with using pull requests that are not immediately merged (the PR can become invalid due to another PR being merged), along with the need to manage dependencies between PRs.

Imagine if you make a pull request A, and then you make a pull request B that depends on A.

Now, while you are waiting for review/having someone review your code, pull request C is merged into master.

As a result you might need to update both pull request A and B to be compatible with C.

---

Considering nixpkgs gets ~500-1200 commits per week, this could quickly become a problem if the PRs remain open for several hours.

[grahamc]

@bergkvist, I feel you're here (sea-lioning whiteknighting? raising unlikely issues that also do not impact you) an issue that has no effect on you. Bitrot happens to all sorts of contributors. It is true. It impacts non-merging contributors more than anyone else, and people with merge access are the least impacted. Being asked to go through PRs is not likely to significantly hinder committers.

[bergkvist]

@grahamc You're right. Thanks for pointing it out. This is an issue which doesn't impact me directly - so I should also not have any impact on this issue. (Because I wouldn't personally feel the pain of having made a bad choice)

On a side-note, I really appreciate the work that you put into maintaining nixpkgs - and for making it the best package manager out there (in my opinion).

[domenkozar]

See https://botsin.space/@complainingaboutmastercommits for some statistics (I haven't verified if those numbers are correct).

[FRidh]

See https://botsin.space/@complainingaboutmastercommits for some statistics (I haven't verified if those numbers are correct).

I am pretty sure those number are incorrect for the reason mentioned in NixOS/rfcs#79 (comment). Same author.

[matthiasbeyer]

See ***@***.*** for some statistics (I haven't verified if those numbers are correct).

They are probably not, but IMO a good approximation. Also,... Squash merges can break master, too, right?

[NeQuissimus]

Considering I have broken master on more than one occasion, I figured I'd chime in :)
I have finally convinced myself to use PRs for everything and from first-hand experience have multiple observations:

• Less breakage
• Availability of @ofborg for test evaluation is huge, I actually found myself adding tests on a regular basis
• There have actually been more people in the community giving feedback, reducing my workload (:pray:)

I think a few questions should be answered to make this process even better (and the RFC mentions these):

• Auto-merging for PRs that have tests
• I have a hard time with the distinction between master, staging, staging-next and equivalent stable branches. What goes where? How often are the branches merged into each other? This may just be my lack of insight... I think we need a way for an automatic label or a bot to ensure changes are sent to the correct target branches
• Is there a way we could ask @ofborg to take a PR and also evaluate against the stable branch(es)? Say I make a PR with kernel updates. If I could just @ofborg stable 20.09 and it duplicates all its checks with the additional set running against 20.09, that would be fantastic. Otherwise the turnaround time would likely double before updates hit stable (Unless we do it entirely in parallel but then we cannot git cherry-pick, can we?)

[matthiasbeyer]

I think we need a way for an automatic label or a bot to ensure changes are sent to the correct target branches

Aren't the tags about how much gets rebuilt by a PR automatically added by a bot? Those could serve well for that purpose, I suppose?

[infinisil]

Direct pushes are now blocked since #249117, and the bot was removed in #249151, so we can officially close this issue now! 🚀