4 ms·

It's an interesting question as to which is more readable: -module_param(max_sets, int, 0600); +module_param(max_sets, int, S_IRUSR | S_IWUSR); Howeve

It's an interesting question as to which is more readable: -module_param(max_sets, int, 0600); +module_param(max_sets, int, S_IRUSR | S_IWUSR); However, I'm missing context. Did someone at Intel really spam 1,285 patches without any prior discussion?

MichaelGG 10y ago
Seems like it. I would hope Intel wouldn't hire someone that would think this is OK ... OTOH see Uber's writeup about how developers can't be expected to think about transactions. Maybe there's some perverse incentive at Intel? Like performance review that considers how many patches you've sent? Or maybe its a broken tool to help submit patches?
- emeraldd 10y ago
  I'd be inclined to think it was a commit per smaller change set and each commit got turned into a separate patch.
  - eridius 10y ago
    It looks like it got turned into one commit per file. Which is pretty atrocious.
    - tedunangst 10y ago
      Isn't "one commit per independent change" common advice in git tutorials? Each file can be changed independently of any other, so taken literally, exactly how you're supposed to do it.
      - eridius 10y ago
        That's wildly incorrect. "One commit per logical change" would mean that every single file modified in this way should have been in a single commit. By your logic, you could never have two files changes in the same commit.
        mappu 10y ago
        > you could never have two files changes in the same commit. We're going OT here, but i don't think that applies to C, where a "logical change" to a function definition may be split over two files (.c/.h).
      - bjackman 10y ago
        In the kernel, the proper practice for sweeping changes like this is one commit per subsystem.
ecma 10y ago
There are some mixed opinions in the maintainers' replies. Steve Rostadt is NACKing all of his patches (amusingly also spamming everyone in the process) since he likes the octal and others are making the same argument. Other again are suggesting more use of S_IRUGO since the author seems to have broken up a large number of octal macros into naive combinations of the S_* macros without much thought for semantic intention. A hardware maintainer encouraged the use of some ATTR_RW macros which indicates that the author didn't necessarily recognise the preferred tropes for various subsystems. The backdoor argument (mentioned in an earlier email in the OP thread) is pretty reasonable to raise. The maintainers seems to be fairly on top of things though, acking and nacking as patches come in but this will take a while to process. I suspect very much that these commits will have to be rewritten to include subsystem labels and more accurate summaries by the maintainers if they want to include them. The patch set as an aggregate whole is garbage.

MichaelGG 10y ago

Seems like it. I would hope Intel wouldn't hire someone that would think this is OK ... OTOH see Uber's writeup about how developers can't be expected to think about transactions. Maybe there's some perverse incentive at Intel? Like performance review that considers how many patches you've sent? Or maybe its a broken tool to help submit patches?

emeraldd 10y ago
I'd be inclined to think it was a commit per smaller change set and each commit got turned into a separate patch.
- eridius 10y ago
  It looks like it got turned into one commit per file. Which is pretty atrocious.
  - tedunangst 10y ago
    Isn't "one commit per independent change" common advice in git tutorials? Each file can be changed independently of any other, so taken literally, exactly how you're supposed to do it.
    - eridius 10y ago
      That's wildly incorrect. "One commit per logical change" would mean that every single file modified in this way should have been in a single commit. By your logic, you could never have two files changes in the same commit.
      - mappu 10y ago
        > you could never have two files changes in the same commit. We're going OT here, but i don't think that applies to C, where a "logical change" to a function definition may be split over two files (.c/.h).
    - bjackman 10y ago
      In the kernel, the proper practice for sweeping changes like this is one commit per subsystem.

ecma 10y ago

There are some mixed opinions in the maintainers' replies. Steve Rostadt is NACKing all of his patches (amusingly also spamming everyone in the process) since he likes the octal and others are making the same argument. Other again are suggesting more use of S_IRUGO since the author seems to have broken up a large number of octal macros into naive combinations of the S_* macros without much thought for semantic intention. A hardware maintainer encouraged the use of some ATTR_RW macros which indicates that the author didn't necessarily recognise the preferred tropes for various subsystems. The backdoor argument (mentioned in an earlier email in the OP thread) is pretty reasonable to raise. The maintainers seems to be fairly on top of things though, acking and nacking as patches come in but this will take a while to process. I suspect very much that these commits will have to be rewritten to include subsystem labels and more accurate summaries by the maintainers if they want to include them. The patch set as an aggregate whole is garbage.