Depressing to see reviewers waste review bandwidth bringing up issues such as "wasted newline" and "incorrect comment format". Do kernel developers not use auto-formatters?
On the contrary, I think the feedback provided was excellent and far better than just saying “Doesn’t conform to our style guidelines, please try again”. Bravo Peter.
This may be someone’s first submission and they may consequently not be aware of style guides, tools which can help lint, etc?
Because bugs due to missing {} happen, and are a royal pain, the rule is that any nontrivial statement; especially any multi-line; must have braces.
Also, given the amount of patches I have to read, uniform style really does matter. Unconventional style breaks the flow and detracts from the important bits.
Also, I'm not aware of a lint like tool that works on diffs. Many of the patches never get further than my MUA.
I think you took my comment as sarcasm, but I quite agree with you; being able to leave out the curly braces was a mistake in the language design. Not as big as some of the others, but costly enough over all these decades.
Personally I prefer to be less strict about style whenever possible, but then I prefer to work in safer languages, and I don't have the same firehose to deal with.
The technical review wasn't technical. It was a human roleplaying as a code formatter. The technical content was entirely found in the comment about ABI compatiblity.
I didn't see any discussion about tradeoffs, alternative approaches, or a survey of what other systems do for this kind of functionality, or detailed benchmark results.
The tone was roughly what I'd want people to give me in a code review -- the only problem is that it was trivial, and could have been summarized as "Fix the style, check it with $tool"
> The technical review wasn't technical. [...] The technical content was entirely found in the comment about ABI compatiblity.
That review also had a comment about an implicit limit on the number of objects, which is caused by a limit on the amount of physically contiguous memory the kernel memory allocator can obtain at once, and a comment that the code being reviewed would allow for a large increase of the reference count of a couple of important structures. Both appear to be very technical comments to me.
In a project like the kernel consistent style is important and the kernel has tools (Coccinelle spatches, checkpatch.pl) to help developers comply with it. There are standards that need to be followed and the bar is the same for everyone.
Because it's not the committer's job to play housekeeper for the contributor. They have enough work to do already, and the contributor is the one who wants their code merged in the first place - they should put in the basic effort to clean their own code before submission, just like everybody else. And because that would introduce some grey areas into the Signed-off-by line[0]. Committers do not want to change submitted code. See the bit about process for maintainers modifying submitted patches and imagine if they had to do that all the time because people can't just follow the damn style guide.
You haven't contribute to a large project or figured out its culture yet, have you? In most major free and open source projects, the default rule is: the entire burdens of correcting any issues, including code-formatting, is the sole responsibility of the contributor, some big projects on GitHub even integrated automatic checking, few people would bother to review a patch if it doesn't fit the coding standard or fails to compile). And no, it's not only used an excuse for disliking one's patches. Even long-term contributors often resubmit some patches to fix their coding styles. If you are a high-profile developer of a subsystem/project, perhaps sometimes you can get a generous help for a free typo/style-correction by the upper level committer (especially when the patch has already went through the review and started moving up, at this point, even the maintainers agree it's pointless to send the patch back), but in general, there's no such a thing.
Do you really suggest that ten reviewers (or however many they may be) do this work each and every one, because it is too much work for the sole submitter to do this once?
It is hard enough to get people to review code as it is. I think everyone would be better off with a little humility and be thankful that other people review their code, even in the cases where the review itself isn't very helpful.
It's the same reason you dress nice and comb your hair for a job interview.
If the developer of the patch couldn't get the easy minor details right before submitting, I wouldn't have much confidence that they spent a lot of effort thinking about the hard, major details either.
I think a law that says speeding is very backwards. When I'm late, details like how far over the speed limit I'm going are a blocker or a waste of time.
The kernel team came up with rules about how code is formatted. If you don't follow the rules, they are under no obligation to allow your code to be merged in to the main repo, and in fact, are within their rights to reject it. I take the initial response more as a "I'm going to let you off with a warning" versus "Here's your ticket, see you in court"
For Linux kernel, the first round of review is always style-checking, it's the standard operating procedure. Ideally, the patch submitter should have already used ./scripts/checkpatch.pl and eliminated all formatting issues, but often there are missed ones, or other style issues not identified by the tools as well.
To me, it serves as a type of virtue signalling. It's kind of interesting to view the issue from a social perspective:
1. It gives a feedback to the committer, shows that your patch has caught the attention of a kernel maintainer, not lost or ignored (Example: last time, I sent a bunch of patches to a subsystem, no reply at all, it turned out that the maintainer was on a vacation. On the other hand, if I received a review on non-conforming code style, I know the maintainer is at least available, and I'm not rejected because I did something seriously wrong).
2. It gives kernel maintainers a chance to immediately expresses objections to your patch, thus affirming the social status and authority of a kernel maintainer (Example: After submitting a few patches, you'll quickly know who's in charge and who has a saying on the development).
3. By doing (2), it also creates a personal connection from the maintainer to the committer, the committer now knows all the sequentially modifications can be CC-ed to maintainer J. Random Hacker for review (although scripts/get_maintainer.pl should always be used, but at least you know who's the most active one).
4. It exerts peer pressure to the submitter to follow the cultural norms, "the system" of the kernel development process, including obeying the Linux kernel coding standard.
5. It creates a system of bureaucracy that could accelerate and mechanize the workflow of a patch-reviewing maintainer (Other examples include pull requests written in a formal, respectful language, often semi-automatically generated, can be compared to the bureaucracy paperwork, e.g. https://lore.kernel.org/lkml/20190731062622.GA4414@archbox/).
6. A lot of the older kernel code has many strange nonstandard coding styles and technical tricks from the early days, which is now discouraged. A strict coding style review prevents any nonstandard practice continues to enter the kernel as new code.
The act of expressing role and power through virtue signalling exists in all organizations. If "the system" itself serves its intended useful proposes without objectionable, serious harms [0], there is no reason to abolish it.
The only problem seems to be frustration over lengthy E-mail exchanges without progress. However, the workflow of Kernel is large, loose, highly asynchronous across different timezones, with a lot of reviewers, some are not even dedicated to the kernel project. Organizing itself already implies a relatively slow pace, so it's not seen as a major problem.
I believe most traditional FOSS project works more or less in the same way. In fact, I think Linux Kernel is actually a lot more open that other similar low-level projects, at least for the "non-core" (not linux-mm) parts, like device drivers.
Finally, I think there are valid criticisms to the traditional model of a FOSS project driven by mails, and many people have attempted to innovate towards a more accessible system of development. GitHub's "Pull Request" proved to lower the barrier-of-entry and boost productivity considerably for small-to-medium projects. And I welcome other innovations if you are starting a new project. On the other hand, the Linux Kernel is now a canonical representation of the "old system" which is very unlikely to change in the next 20 years. My recommendation is: Don't waste your energy to attack the old systems, instead, learn from all major projects and study their workflow and governance, and see if you can invent something new, we need a lot of innovation).
[0] Verbal abuses are criticized as a problem of this system, but by itself, it's not a part of the workflow, using what words is more closer that a matter of personal choice (so yes, one could say harsh criticisms is a greater problem in hacker culture, not only a workflow problem, it can be seen on mailing list, on online forums, IRC, or even offline). Also, Linus Torvalds recently changed his behavior under external pressure.
It also creates a filter for investment in the patch. If the submitter doesn’t bother to do a second round for trivial revisions, it would have been a waste of time for the maintainer to read the patch deeply.
This may be someone’s first submission and they may consequently not be aware of style guides, tools which can help lint, etc?