On Sun, Feb 23, 2020 at 8:38 PM Allison Collins <allison.henderson@xxxxxxxxxx> wrote: > > On 2/23/20 5:42 AM, Amir Goldstein wrote: > > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins > > <allison.henderson@xxxxxxxxxx> wrote: > >> > >> Factor out new helper function xfs_attr_leaf_try_add. > > > > Right. This should be the subject. > > Not factor out xfs_attr_leaf_addname helper. > > > >> Because new delayed attribute > >> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname > >> that we can use, and move the commit into the calling function. > > > > And that is a different change. > > It is hard enough to review a pure factor out of a helper. > > Adding changed inside a pure re-factor is not good. > > > > Belongs to another change - move transaction commit to caller. > > Yes, this came up in the last review, but the reason I avoid it is > because I think the transaction looks weird in either helper. The > reason the roll is here is because there is no room to add the attr as a > leaf, and so we need to hand it off to the node subroutines. But that > seems like something that should be managed by the caller, not leaf > helpers. So I was concerned that separating the split and the hoist > would generate more weird looks from people trying to understand the > split until the see the hoist in the next patch. If people really think > it looks better that way, I can split them up. But I know folks have a > tough enough time trying to recall the discussion history, so I'm trying > to avoid confusion of another type :-) > > Thoughts? > It's fine, so long as you document it properly in commit message. See, we are so bad in this review process, that we rely on humans to verify that logic preserving re-factoring patches are indeed logic preserving. This is so lame to begin with, because there are static checker bots out there that would gladly do that work for us :) ... if we only annotated the logic preserving patches as such. In the mean while, while we are substituting the review robots, the least a developer could do is not call out a patch "re-factoring" and sneak in a logic change without due notice to reviewers. Thanks, Amir.
