On Tue, Jun 28, 2011 at 2:41 AM, Ted Ts'o <tytso@xxxxxxx> wrote: > On Tue, Jun 21, 2011 at 12:39:27PM +0300, amir73il@xxxxxxxxxxxxxxxxxxxxx wrote: >> From: Amir Goldstein <amir73il@xxxxxxxxxxxx> >> >> The next patch is going to move ext4_ind_ functions to >> indirect.c. First, we clone the file from inode.c and >> only leave code that is going to be duplicated in both files. >> This should keep the deleted lines count from inode.c in the next >> patch the same as the added lines count to indirect.c. > > I really dislike cloning functions. This becomes a maintenance > headache, since bugs that get fixed in one file might not get > propagated to another. Since I need to manually move all of the > functions to verify nothing else changed in patches that do massive > code movement, I used the first two patches in your patch series, but > replaced the last two patches in the patch series, and replaced it > with the following: > > A) Move __ext4_check_blockref() to fs/ext4/block_validity.c and > declare it extern. > > B) Move ext4_truncate_failed_write() and blocks_for_truncate() to a > new function, fs/ext4/truncate.h. Nice cleanups! > > C) Movement of indirect-related code to fs/ext4/indirect.c. > Hmm.. in my patch the deleted lines from inode.c matched the added lines in indirect.c (-1 newline at end of file). Your patch has: fs/ext4/indirect.c | 1510 ++++++++++++++++++++++++++++++++++++++++++++++ fs/ext4/inode.c | 1486 --------------------------------------------- This accounts for 28 lines added by header comment and includes in indirect.c -1 newline at end of file, so where did 3 more lines go? I am guessing these are just deleted newlines, but have no way of knowing. Have you made other (maybe checkpatch) changes while moving or just removed 'static' from function declarations? I am asking just in case I get 'git blamed' for it some day ;-) Cheers, Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
