I see that these "cleanup" patches are not bringing much value, bust I also don't see why fixing spelling mistakes or log messages is considered as harmful, KPI boosting or not.
The maintainer even said if someone else sent those patches it would be OK, but not if Huawei employees do it. If they distrust Huawei so much, why not just ban them from committing, the same way they did recently with university "security researchers".
It's not about security. It's about wasting reviewers' time. I too would be annoyed if someone was submitting, say, whitespace-only changes, or similar cleanup, non-functional changes - I still have to review them and my time would be better spent looking at actually meaningful changes that make the product better. It's OK if someone is just learning the ropes with an easy change, but this seems to be more like people trying to game a metric by submitting lots of small changes, which wastes the reviewers' time for no actual user benefit.
I think there was some medium post by a guru where he claimed the best way to get experience was to submit as many spelling error pull request as possible.
That way he can throw in his resume that he's contributed to a dozen or so open source projects. Given how broken the hiring process is, it wouldn't surprise me if this works
There is a certain balance to this however. Where I work we generally have a rule of no formatting changes mixed in with real code changes to make reviewing easier. It's a huge pain to see a ton of white space fixes mixed in with logic changes. As long as they're separate commits in the same pull request it's usually OK. We also generally don't have formatting-only pull requests because it just adds overhead and indirection to git-blame, and doesn't add value to the end product.
Would you rather spend 5 minutes to read a cover letter and code, or just read the diff see word_a has been changed to word_b in comments? Trivial typo or cosmetic changes should stay small, and can be approved in a few seconds.
These small patches create some overhead for maintainers. He actually said in the follow up message that grouping them together is fine.
Please at least merge all those small fixes into a larger patchset, and with a good cover letter to explain the reason (and auto-tool to do the change if possible) for all the involved maintainers, so that all of us are on the same page.
I understand the overhead, but it seems like bundling lots of unrelated cleanup fixes into one large commit would make it easier to sneak in something nefarious.
They’re still going to be reviewed, and if the cover letter says “typo fixes” and one of the patches fixes more than a typo, that would be an instant red flag. A single bigger patch set is faster and easier to review.
My understanding is that they're not necessarily asking for the commits to be squashed, they're asking for them to be submitted together as a patch series.
I'd argue it would make it LESS likely; seeing a hundred "cleanup" merge requests that are all basically the same thing vs a single one where something more than just cleanup would stand out.
>> I see that these "cleanup" patches are not bringing much value, bust I also don't see why fixing spelling mistakes or log messages is considered as harmful, KPI boosting or not.
My best guess: It creates a small amount of work for a maintainer to review and merge. This is worthwhile if a new contributor is learning their way around, and getting new contributors is very important to an OSS project. To have a large company submitting a bunch of these is a distraction for maintainers who have better things to do than support someone trying to inflate their metrics.
It's true that it creates an overhead for maintainers. But, on the other hand, maybe Linux process needs to be improved to relief the maintainers from having to review every single patch.
Linux as a project is big enough, and if maintainers are the bottleneck, maybe it's time to have sub-maintainers to whom such tasks could be delegated.
See, this will just cause additional communication overhead for the maintainers, too. And for what benefit, just to accommodate the behavior we see here?
It's likely the context of available time and resources that matters here. Huawei has the ability to contribute a lot, but instead sends an experienced, highly paid person around to hammer in a nail that was sticking out slightly and calls you over (when you could be doing something more productive) to high five a job well done. This would be appropriate for an beginner apprentice.
Yeah, it might've been slightly annoying to someone, but it's a visible waste of time overall.
I think the concern here is that at huawei there's some kernel patch metric, or incentive, or something like that. And following Goodhart's law... "When a measure becomes a target, it ceases to be a good measure."
So now there's a target that costs maintainers time, possibly lots of time, and costs huawei nothing.
There's really no control here to dial it back other than to ask 'please don't do this'.
Is this an issue with the workflow that the kernel uses in terms of time spent per patch? In most workflows this seems like it'd take under a minute to review, approve and merge each one although the CI/CD might add some computing cost.
Looks to me like the problem is not the content of the patches, but the way they are submitted as many small patches all from the company. I suspect the maintainers wouldn't be as concerned if all of the contributes were independent (no "@huawei.com" address) or if they bundled the patches together to reduce overhead.
In general fixing spelling or logging errors are P4 or P5 at best so they should only be fixed if something more important is touching those files. That how I've always viewed it.
The dialog between the author of this message the responses on the mailing list add some clarity. There's nothing inherently wrong with providing cleanup fixes to this kind of project, but submitting those as individual merge requests instead of one grouped patch is just serving to increment the developer's "number of patches submitted" KPI at the expense of the time spent by maintainers reviewing every individual patch.
Right? I am not really sure I see the problem. It seems to me like Huawei is just submitting clean-up patches and the maintainer has higher expectations of what patches they should be submitting. Granted I am not a developer not a maintainer of any project so that might be why it is not obvious.
If there’s a pattern of them submitting a ton of these kind of patches, that could be viewed as gaming a metric. Huawei might, for example, be assessing kernel developer performance partially on # of upstreamed commits, and I guess that’s what is being suggested here.
The easy solution would seem to be for the person accepting the patch to check a box that says 'this patch materially improved the codebase'. That would put an instant stop to gaming the KPIs.
I would agree with you. Those commits would be the easiest one to merge. How difficult is it to approve a typo correction? Saying a typo correction is not productive is actually the opposite: it takes very little time to review and approve. I don't know why people are mad that these commits are not from a student.
When people make trivial typo PRs on my open source projects I say thanks very much, close their PR, and make the change in a commit under my name. This way, if their interest is in improving the project then it worked. And if their interest is in contributions then it works also because it doesn't pollute their contribution history with trivial changes that aren't real contributions.
Sorry, you missed my point. I'm talking about people who tour around GitHub making PRs comprising a single letter typo charge in order to bolster their contribution statistics. For example the last one I got was changing Github to GitHub in one location. I have no time for that whatsoever. I'm otherwise an extremely friendly and respectful maintainer, including with beginners, of course, and my project has various repeat contributors; I'm sorry I can't point you to it to convince you of that. Your comment is a misjudgment.
Maybe it's easy to feel it's disrespectful because they've made social networks out of contributing to repositories.
Accreditation for the modification of petty and trivial issues is a nice and decent gesture, but overall unworthy of a paper trail binding me to any random none-of-my-business project for such a drive-by PR.
Personally, I would feel more comfortable sending that kind of inane PRs if the parent's way of merging was the standard one, but I know it isn't and therefore opt to keep separate accounts to compartmentalize (which unfortunately increases friction to such contributions).
> Accreditation for the modification of petty and trivial issues is a nice and decent gesture, but overall unworthy of a paper trail binding me to any random none-of-my-business project for such a drive-by PR.
I don't know about others, but before investing significant time in a project, my first action will be to read through the documentation, then the code (and accompanying comments), and then I'll make minor fixes along the way.
After this, I'll submit a PR with my changes. If a maintainer were to close my patch and then proceed to commit the exact same changes under their own name, I'd never contribute, use, or improve that project again. Why would I waste my time?
That kind of behavior displays an immense lack of respect for contributors.
The maintainer even said if someone else sent those patches it would be OK, but not if Huawei employees do it. If they distrust Huawei so much, why not just ban them from committing, the same way they did recently with university "security researchers".