Say I encountered a PR that added some code that there is a consensus is wrong - either according to the Nixpkgs manual, or pkgs/README.md. Or for example someone marked a package as broken and didn’t write a comment or in the commit message explaining why.
And say I encountered the above change after a while, and I write a comment in the PR, and the author of the offending commit ignores my comment and doesn’t fix the issues they have presented.
It may also be that the author of the PR didn’t merge the wrong commit, but some committer merged it, and they too don’t respond to comments about their mistake, and they don’t make an effort to fix the mistake.
This behavior is severe IMO, and I think that such a committer should be punished somehow. What do you think?
If I understand the content of this resolution correctly, the committer delegation team is responsible for “changing” the committer list. So complaints about committers’ lack of technical and communication skills should be resolved by them, and decide whether to remove problematic committers. Of course, reports of their words and deeds not being in compliance with the CoC should still be handled by the moderation team.
I personally prefer to first communicate with the contributor before escalating, but I understand that some fear retribution. We really should provide some anonymous means of reporting poor conduct.
After discussion with one of the moderators, I was convinced that this is out of their domain, as this is not an inappropriate behavior on a personal level, but rather on a professional level, and hence this is the domain of the governance team.
The whole point of my complaint is the lack of communication, much more then I’m bothered by the mistake itself.
I think it should be a PR just like any other PR (I’ve seen such PRs in the past). However there is no official “governance” team so it seems, so you’d have to hope someone confident enough to approve such a PR would stumble upon it. Perhaps linking the PR in this thread might be a good idea as people hear have shown that they care enough about such experiences so they might be able to review such a PR.
I was running a script that traverses every commit in nixpkgs since 24.05 to run some stuff, and I uncovered this PR: briar-desktop: 0.5.0-beta -> 0.6.0-beta by s1ls · Pull Request #260649 · NixOS/nixpkgs · GitHub. It contains a merge commit …which obviously isn’t supposed to be there. It’s a simple oversight while merging, compared to the other things I notice going around in PRs, but it’s a critical one.
It was notified to the committer in the PR, and I don’t mean to escalate this PR in particular, but instead use this to put my point that we really need a place to report these things. It could be as simple as a thread similar to Breaking changes announcement for unstable or PRs ready for review _, with comments linking to offending PRs.
What the outcome should be can be discussed by the delegation team, but these things need to be reported.
EDIT: Even if not “punishments” (since that’s quite the debate), having a mechanism of posting these things on some thread can sub-consciously pressure committers to be more diligent and pay undivided attention to what they’re doing. This is really a win-win situation.
I assume you meant “it’s not a critical one”? Because merging with a merge commit is effectively non-consequential, so much so that apparently nobody even noticed for 6 months!
It’s pretty clear to me that that was just an honest mistake, which is very different from the scenario this thread was created for.
Mistakes are natural, everybody makes them, it’s part of what makes us human, and they help us improve. We should definitely inform people kindly when they make a mistake, so they can learn from it and if necessary take actions to reconcile it. And if we see certain mistakes occurring more often, we should figure out the underlying issue (not enough mentoring? Poor onboarding docs? Too much to do?) and address that (improve mentoring and docs? Add more automation and empower more people?). Mistakes are a good thing and we should positively embrace them.
This however goes in the opposite direction by focusing on negativity: It publicly shames people for having made mistakes and creates fear of making mistakes because you don’t want to end up being humiliated for it. This is not something anybody should be subjected to, not even on a payroll, but it would drive especially volunteers away from contributing (“Why would I spend hours of my free time merging PRs if I get shamed for every mistake?”, “The less I contribute, which I have no obligation to do, the less mistakes I make”).
In my community building experience, my rule of thumb is that if there’s a positive incentive that would do the equivalent thing to a negative one, it’s almost always better to prefer the positive one. People usually respond better if they feel like they own the result rather than feeling like they’re going to be punished for every last thing. The latter makes everything feel unnecessarily adversarial.
I agree. And in hindsight, the PR I linked was a really bad example for what I meant to convey.
Perhaps my proposed solution is inherently flawed, I agree with most of what you’ve mentioned. My focus was more on “something needs to be done”, which I really don’t see anything about. An example is this thread I opened with my last comment being regarding onboarding, as you mentioned.
Of course, things take time; that unfortunate PR happened to be my tipping point with all of the other issues which go beyond one-off mistakes (they are very much systemic).
I don’t mean to be a misanthrope and I hope my words aren’t read that way either (given they sometimes unintentionally sound that way ). We’re all part of the same team trying to make this better
Maybe we should try for more optimistic merging? Social Architecture lays it out:
It turns out that accepting imperfect patches rapidly, which I call “optimistic merging”, works better all-round than insisting that contributors deliver perfect work.
Standard practice (Pessimistic Merging, or PM) is to wait until continuous integration testing (CI) is done, then do a code review, then test the patch on a branch, and then provide feedback to the author. The author can then fix the patch and the test/review cycle starts again. At this stage the maintainer can (and often does) make value judgments such as “I don’t like how you do this” or “this doesn’t fit with our project vision.”
In the worst case, patches can wait for weeks, or months, to be accepted. Or they are never accepted. Or, they are rejected with various excuses and argumentation.
… what does this sound like?
So, I’d prefer hedging on slight process imperfection to trim the backlog down. Nixpkgs is by definition slightly controlled chaos, if someone makes a mistake like a merge commit we should ask ourselves what it’s actually breaking and if maybe the commit added some more value than the mistake.
That entire chapter of Social Architecture is amazing, by the way. It focuses on how the vast majority of patches are made in good faith and communities should focus on accepting them, even if there are mistakes. Because, otherwise:
It tells new contributors, “guilty until proven innocent,” which is a negative message that creates negative emotions. Contributors who feel unwelcome will always look for alternatives. Driving away contributors is bad. Making slow, quiet enemies is worse.
Food for thought. If the same mistakes keep happening, it probably points to a communication issue of some sort. (Of note, I like docs that say “read all of this before requesting access” - like the community builders. Maybe there’s a secret word at the end of very important docs that you PM to the team granting access to prove that you read it… )
Edit to add: by-name is a great example of CI-based optimistic merging in nixpkgs.
Hmm I’m mostly agreeing with the optimistic approach, and when I from some reason encountering a stale PR which I find important, I am much less strict in my demands for perfection from the PR author. Sometimes, if there is a merge conflict because the PR was stale for a long time, I ask the contributor if they would like to refresh the PR and I’m still very forgiving in my demands. So I’d say it is a question of balance. Also, from a 1st time contributors that are active and fast to reply to reviews, it is natural to strive for perfection, as this is a way to onboard people to the standards of the community.
Also, it seems that this text mainly criticizes CI dependent practices, and I have 1 comment to say about that: Unlike most projects such as programming languages’ libraries etc, in our case, Nixpkgs does not provide certain functionality that need to pass tests that prove that the desired behavior of the functions is achieved, but rather, it should mainly pass evaluation, which would be very annoying to fix after the PR is merged, so I’d consider this a hard requirement for PRs.
The other things that our CI does like labeling and building the directly changed packages (according to commit messages’ prefixes), is definitely not a must, even in today’s culture. The only real bummer, which takes way too much time (more then 15 hours from what I see now!), is the evaluation and checking which paths are changed just from evaluation with and without the PR - that definitely has got to change, because even when a PR looks good, it can take more then a day for this important part of CI to be green. Also the labeling depends on this, and knowing that a PR doesn’t cause changes to above 1000 packages is also a hard requirement IMO.
The bottom line is, that faster CI is extremely important .
). We’re all part of the same team trying to make this better 
)
.