Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/20-bug856400

Revision history for this message
Alexey Kopytov (akopytov) wrote :

Sergei,

Some comments and questions:
  - conflict in xtrabackup.cc
  - typo in the comments for cleanup_dir_recursively() (double "directory")
  - wrong bug reference in the test case comments
  - there's no need for "\/+" in the following code:

:+ (my $rel_path = $File::Find::name) =~ s/^$copy_dir_src\/+//;

  - can you clarify the purpose of the code in lines 50-52?
  - error handling for unlink() would be nice
  - did you take the comment in lines 125-128 into account?
  - a few lines are longer than 80 chars, try:
: bzr diff -c-1 | grep -0 '^+' | expand | awk 'length > 80'
    or better yet, try to use a good text editor that can be configured
    to highlight such lines
  - the check_newer argument is not used. does it really make sense to
    add some _new_ code to support it? Wouldn't it be better to get rid
    of it?
  - in the test, do we really have to restart the server between full
    and incremental backup, or it's just a copy-paste from another test?

review: Needs Fixing

« Back to merge proposal