complaining about stupid stuff
complaining about stupid stuff
Posted Sep 10, 2024 9:06 UTC (Tue) by error27 (subscriber, #8346)Parent article: Testing AI-enhanced reviews for Linux patches
Good kernel developers are always people who are very obsessed with details. They're not going to agree which details are important or even which ways are correct. You have to take the good with the bad. Most maintainers are very sensible people, but you can't hire the most detail oriented people you can find and then ask them to change their personalities. You have to accommodate maintainers and their foibles. The slides talk about "the first letter in the subject should be capitalized" and that's true sometimes. Other times it is the opposite. Some maintainers have claimed that it's American to capitalize the first letter and not how they were taught in school. As a developer, it's fine. Adapt.
The person I was talking to has been banned from a lot of subsystems. He's obsessed with the wrong parts like minor typos in the commit message or imperative tense. He never points out bugs in the patches. His comments are so confusing so it ends up generating a pointless thread. We all have to read this pointless thread and do damage control because a lot of his instructions are weird and wrong.
Occasionally, he will say things which are correct but even there, what he's pointing out is obvious. Everyone with eyeballs can see there is a missing Fixes tag. It's only newbies who leave off the Fixes tag and they assume he knows what he's talking about so they send a v2 of the patch. Now the discussion about the actual code is spread over two different threads. So even when he's correct, it's not helpful. Why doesn't he look up the code which introduced the bug and add that person to the CC list? A useful comment about Fixes tags would be, "I'm not qualified to review this patch, however you're missing a Fixes tag. Fixes: 123412341234 ("blah blah blah"). I have added Jane Doe to the CC list because she wrote that commit. After the maintainer has reviewed this patch, then please, resend with the Fixes tag."
I think that newbie reviewers are excited to spot a mistake in someone's patch. But once you've reviewed enough code, you realize that none of it is perfect. There are magic numbers. People hard code error codes instead of propagating them. There are if statements which would be more readable if they were reversed. There are checks for things which can never happen. The code uses "rc" in some functions and "ret" in other functions. There are header files which don't need to be included or the header files not in alphabetical order. The label names could suck.
Obviously bugs are not allowed. And there is a minimum level of good style which is required. But I'm not going to nitpick the code to death. I'm mostly looking for effort. If people are trying and they stick around to fix bugs then we're going to move in a good direction.
