On Mon, Feb 24, 2020 at 9:09 AM Allison Collins <allison.henderson@xxxxxxxxxx> wrote: > > > > On 2/23/20 11:38 PM, Amir Goldstein wrote: > > 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 :) ... > I didnt know there were tools out there that did that. They sound > helpful though! > > > 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. > Ok, what if the title were to say something like: > xfs: Split out node handling from xfs_attr_leaf_addname > > Or maybe just "Split apart xfs_attr_leaf_addname"? Does that sound like > it would be a better description here? > Sure, both are fine. All I'm saying is that "factor out" is a well established keyword that shouldn't be abused. Thanks, Amir.
