On Mon, Jun 15, 2020 at 4:50 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Thu, Jun 11, 2020 at 10:07:09AM -0400, Brian Foster wrote: > > > > TBH, I think this patch should probably be broken down into two or three > > independent patches anyways. > > To what end? The patch is already small, it's simple to understand > and it's been tested. What does breaking it up into a bunch more > smaller patches actually gain us? > > It means hours more work on my side without any change in the end > result. It's -completely wasted effort- if all I'm doing this for is > to get you to issue a RVB on it. Fine grained patches do not come > for free, and in a patch series that is already 30 patches long > making it even longer just increases the time and resources it costs > *me* to maintian it until it is merged. > This links back to a conversation we started about improving the review process. One of the questions was regarding RVB being too "binary". If I am new to the subsystem, obviously my RVB weights less, but both Darrick and Dave expressed their desire to get review by more eyes to double check the "small details". To that end, Darrick has suggested the RVB be accompanied with "verified correctness" "verified design" or whatnot. So instead of a binary RVB, Brian could express his review outcome in a machine friendly way, because this: "TBH, I think this patch should probably be broken down..." would be interpreted quite differently depending on culture. My interpretation is: "ACK on correctness", "ACK on design", "not happy about patch breakdown", "not a NACK", but I could be wrong. Then, instead of a rigid rule for maintainer to require two RVB per patch, we can employ more fine grained rules, for example: - No NACKs - Two RVB for correctness - One RVB for design - etc.. Also, it could help to write down review rules for the subsystem in a bureaucratic document that can be agreed on, so that not every patch needs to have the discussion about whether breaking this patch up is mandatory or not. There is a big difference between saying: "this patch is too big for me to review, I will not be doing as good a job of review if it isn't split into patches" and "this patch has already been reviewed, I already know everything there is to know about it, there are no bisect-ability issues, but for aesthetics, I think it should be broken down". I am not saying that latter opinion should not be voiced, but when said it should be said explicitly. High coding standards have gotten xfs very far, but failing to maintain a healthy balance with pragmatism will inevitably come with a price. Need I remind you that the issue that Dave is fixing has been affecting real users and it has been reported over three years ago? Thanks, Amir.
