> On Jul 29, 2022, at 10:31 AM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: > >> 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. I should say that the issue for me was bisectability, it was easy enough to review the patch. -- Chuck Lever
