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?
Thanks!
Allison
Thanks,
Amir.
