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.
