Re: [PATCH v2 00/29] xfsprogs: Drop the 'platform_' prefix

[Pavel Reichl <preichl@redhat.com>]

On 8/7/21 1:05 AM, Dave Chinner wrote:
> On Fri, Aug 06, 2021 at 11:22:49PM +0200, Pavel Reichl wrote:
>> Stop using commands with 'platform_' prefix. Either use directly linux
>> standard command or drop the prefix from the function name.
> Looks like all of the patches in this series are missing
> signed-off-by lines.  Most of them have empty commit messages, too,
Sorry about the missing signed-off-by, I really need to have a
check-list before posting patches...or send more patches :-).
> which we don't tend to do very often.
>
>>   51 files changed, 284 insertions(+), 151 deletions(-)
> IMO, 29 patches for such a small change is way too fine grained for
> working through efficiently.  Empty commit messages tend to be a
> sign that you can aggregate patches together.... i.e.  One patch for
> all the uuid changes (currently 7 patches) with a description of why
> you're changing the platform_uuid interface, one for all the mount
> related stuff (at least 5 patches now), one for all the block device
> stuff (8 or so patches), one for all the path bits, and then one for
> whatever is left over.
OK, I'll do this in the next version.
>
> Every patch has overhead, be it to produce, maintain, review, test,
> merge, etc. Breaking stuff down unnecessarily just increases the
> amount of work everyone has to do at every step. So if you find that
> you are writing dozens of patches that each have a trivial change in
> them that you are boiler-plating commit messages, you've probably
> made the overall changeset too fine grained.
OK, sincerely thank you for the 'rules-of-thump'. However, In the
first version of the patch set I grouped the changes into way less patches
and passed along a question about the preferred granularity of patches
and got the following answer:

>   * What would be best for the reviewer - should I prepare a separate
>   patch for every function rename or should I squash the changes into
>   one huge patch?
> One patch per function, please.

However, I agree that I should have mentioned that some patches would
be too small and not blindly follow the request...I'll do better next
time.
>
> Also....
>
>>   libxfs/init.c               | 32 ++++++------
>>   libxfs/libxfs_io.h          |  2 +-
>>   libxfs/libxfs_priv.h        |  3 +-
>>   libxfs/rdwr.c               |  4 +-
>>   libxfs/xfs_ag.c             |  6 +--
>>   libxfs/xfs_attr_leaf.c      |  2 +-
>>   libxfs/xfs_attr_remote.c    |  2 +-
>>   libxfs/xfs_btree.c          |  4 +-
>>   libxfs/xfs_da_btree.c       |  2 +-
>>   libxfs/xfs_dir2_block.c     |  2 +-
>>   libxfs/xfs_dir2_data.c      |  2 +-
>>   libxfs/xfs_dir2_leaf.c      |  2 +-
>>   libxfs/xfs_dir2_node.c      |  2 +-
>>   libxfs/xfs_dquot_buf.c      |  2 +-
>>   libxfs/xfs_ialloc.c         |  4 +-
>>   libxfs/xfs_inode_buf.c      |  2 +-
>>   libxfs/xfs_sb.c             |  6 +--
>>   libxfs/xfs_symlink_remote.c |  2 +-
> Why are all these libxfs files being changed?

I believe this is because of patch #6 - xfsprogs: Stop using 
platform_uuid_copy()

Here I dropped the usage of platform_uuid_copy() even in libxfs by:

1) removing the uuid_copy() macro that was implemented by calling
    platform_uuid_copy() in libxfs/libxfs_priv.h

  -#define uuid_copy(s,d) platform_uuid_copy((s),(d))

2) using directly standard uuid_copy() instead
    Which resulted into plenty of trivial changes all over libxfs, the
    core of the changes being that uuid params are passed-by-value to
    standard uuid_copy(), but to libxfs' uuid_copy() it is
    passed-by-reference.

    E.g.
- uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
+               uuid_copy(agf->agf_uuid, mp->m_sb.sb_meta_uuid);
> Cheers,
>
> Dave.
Hi Dave,
Thank you for you comments, please see reactions above.
Have a nice day.