> On Jul 28, 2022, at 9:27 PM, NeilBrown <neilb@xxxxxxx> wrote: > > On Fri, 29 Jul 2022, Chuck Lever III wrote: >> >>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@xxxxxxx> wrote: >>> >>> Also move the 'fill' calls closer to the operation that might change the >>> attributes. This way they are avoided on some error paths. >>> >>> For the v2-only code in nfsproc.c, drop the fill calls as they aren't >>> needed. >> >> This feels like 3 independent changes to me. At least the v2 change >> should be moved to a separate patch. Relocating the "fill attrs" calls >> seems like it could cause noticeable behavior changes, so maybe it >> belongs also in a separate patch? > > Three intimately related changes that could be applied in sequence. > What would be gained by having separate patches? > To my mind, the primary issue is review effort. Mixing unrelated > changes make review of each change harder, so keep them separate. > Mixing related changes is less of a problem as the abstraction that you > need to keep front-of-mind are fewer. > On the flip side, every patch added increases the review burden. If I > wanted to move all calls to foo(), I wouldn't have one patch for each > call. > I think that patch is easy to review as it stands, but if you think not > I guess it could be split. > > Allowing bisect to isolate the problem precisely is another reason for > keeping patches small. If a bug were found to be caused by this patch I > doubt it would be at all hard to localise which part of the patch caused > it. I adjusted the patch description and left the content as a single patch. I was initially confused by "drop the fill calls"... the patch is not physically removing code. -- Chuck Lever
